Skip to content

Conversation

@pitabwire
Copy link
Owner

No description provided.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @pitabwire, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue where Hydra was not correctly opening or utilizing enrichment paths by updating its core configuration. The changes primarily involve adding specific webhook endpoints for token and refresh token enrichment, alongside other detailed OAuth2 settings to bolster security and feature completeness. Additionally, minor cleanup was performed by removing redundant logging statements from the server's HTTP/2 setup.

Highlights

  • Hydra Enrichment Path Configuration: This pull request introduces new configuration settings within frametests/deps/testoryhydra/hydra.go to enable and define enrichment paths for Hydra. Specifically, refresh_token_hook and token_hook are now configured to point to local webhook endpoints, directly addressing the issue of enrichment paths not being utilized.
  • OAuth2 Configuration Enhancements: Several other OAuth2-related settings have been added to the Hydra configuration, including hashers (bcrypt, pbkdf2), pkce enforcement, client_credentials defaults, and grant settings for JWT and refresh tokens, enhancing the overall security and functionality of the OAuth2 setup.
  • Logging Verbosity Reduction: Two informational log messages related to H2C (HTTP/2 without TLS) status have been removed from server.go, streamlining the server's log output without impacting functionality.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@pitabwire pitabwire merged commit c3d8857 into main Nov 20, 2025
2 of 4 checks passed
@pitabwire pitabwire deleted the feat/pbvalidation-struct-validation branch November 20, 2025 06:13
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Ory Hydra configuration to enable token enrichment webhooks, which resolves an issue with enrichment paths not being available. The changes also include several other Hydra configuration updates and the removal of a couple of log messages from the server startup logic.

My review focuses on the security implications of the new Hydra configuration settings. I've identified a few areas of concern:

  • The PBKDF2 iteration count is critically low, which significantly weakens password hashing.
  • The JWT maximum time-to-live is very long, increasing the window of opportunity for misuse if a token is compromised.
  • Internal errors are exposed, which is risky for production environments.

I've provided specific suggestions to address these points. While some of these settings might be acceptable for a test environment, it's crucial to follow security best practices to avoid accidentally promoting insecure configurations. The changes in server.go seem unrelated to the main purpose of this PR; it's generally better to keep pull requests focused on a single concern.

bcrypt:
cost: 10
pbkdf2:
iterations: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The number of iterations for PBKDF2 is set to 1. This is critically low and provides almost no resistance to brute-force attacks, making the hashed values insecure. While this is for a test environment, using such a weak setting is a dangerous practice. It is highly recommended to use a much higher iteration count. For example, OWASP recommends values in the hundreds of thousands for production. For testing, a value like 10000 would offer a better balance between security and performance.

Suggested change
iterations: 1
iterations: 10000

jwt:
iat_optional: false
jti_optional: false
max_ttl: 720h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The max_ttl for JWT-based grants is set to 720h (30 days). This is an exceptionally long lifetime for a token, which significantly increases the security risk if the token is compromised. It is a security best practice to use short-lived tokens. Consider reducing this to a much shorter duration, such as 1h, to limit the window of exposure.

Suggested change
max_ttl: 720h
max_ttl: 1h

grace_period: 1h
refresh_token_hook: http://127.0.0.1:3000/webhook/enrich/refresh-token
token_hook: http://127.0.0.1:3000/webhook/enrich/token
expose_internal_errors: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Setting expose_internal_errors to true can be useful for debugging in development or test environments. However, it poses a significant security risk in production by potentially leaking sensitive information like stack traces or internal application details. Please ensure this setting is disabled in any production configuration to prevent information disclosure vulnerabilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant