feat: OIDC auth#283
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add authenticateWithOidcToken function that validates JWT tokens signed by the data proxy's OIDC provider, checking signature, issuer, audience, and expiry before resolving the subject claim to a UserSession. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
backend roles will be making requests as either organizations or users
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…e handling and directory navigation
… for file path When the server-side HEAD check fails and falls through to the client-side directory listing with a file path (e.g. catalog.json), the listing returns empty. Recursively render with the parent prefix so the user sees the file in context instead of an infinite loading state.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job PR Review: feat/OIDC auth
|
|
Claude finished @alukach's task in 2m 56s —— View job PR Review: feat/OIDC auth
Overall this is a solid implementation of OIDC-based data proxy authentication. The test coverage is good and the architecture is sound. Below are issues found, roughly ordered by severity. Security1. When an // Current behaviour — falls through silently
if (authorization) {
const oidcSession = await authenticateWithOidcToken(authorization, audience);
if (oidcSession) return oidcSession;
// ← returns null here would be more defensive
}
return getPageSession();Consider returning 2. Error responses include raw Hydra response bodies throw new Error(`Login accept failed: ${loginAcceptResp.status} ${await loginAcceptResp.text()}`);
throw new Error(`Token exchange failed: ${tokenResp.status} ${await tokenResp.text()}`);Hydra error bodies could contain sensitive request parameters or internal details. These get propagated through 3. Redirect target not validated before following When following the post-login redirect, the URL is resolved relative to const resolvedUrl = new URL(location, loginRedirect).toString();
const followResp = await fetchWithCookies(resolvedUrl, cookieJar, { redirect: "manual" });If Hydra is misconfigured or compromised, this could follow redirects to unexpected hosts while carrying the session cookie jar (and potentially the admin API key via 4. /** @internal Exposed for testing — override to supply a local JWKS resolver. */
export function _setJwks(fn: ReturnType<typeof createRemoteJWKSet> | null) {
jwks = fn;
}This is exported from a production module, allowing any importer to replace the JWT verification key resolver. TypeScript's 5. Regex-based STS XML parsing The XML is parsed with regex. If credential values contain XML-escaped characters (e.g. Performance / Architecture6. export default async function AppLayout({ children }: AppLayoutProps) {
const session = await getPageSession();
const isAuthenticated = !!session?.identity_id;
return (
<ProxyCredentialsProvider isAuthenticated={isAuthenticated}>For every authenticated page load — account pages, settings, search results, etc. — this triggers the full Hydra OAuth2 authorization code flow (4–6 HTTP calls) on component mount. This is a meaningful latency hit for non-product pages. Consider moving the provider to the product-specific layout ( 7. The credentials are stored in React state. Every time the provider re-mounts (e.g. navigating away and back), the full Hydra flow runs again even if the previous credentials are still valid. The proactive refresh on expiry is a good touch, but consider persisting credentials in Code Quality8. Unused
9. Duplication between Both clients implement the no-op signer pattern and S3 list/get logic. The split is justified (server vs. browser environment), but the unsigned-request approach in 10. if (isEmpty && prefix) {
return <ProductFileBrowser ... prefix={parent} ... />
}Each recursion strips one path segment, so it will terminate at an empty prefix. But each step is a new S3 request, and a deeply nested path would make N network calls before showing content. A flattened loop or resolving parent server-side would be more predictable. 11. The same config key drives OIDC token verification (issuer claim) and STS endpoint construction. This is fine if the data proxy hosts both, but it couples the two concerns to a single env var. If the STS and JWKS endpoints ever diverge, this will require a config change. A comment explaining the coupling would help future maintainers. 12. Missing The Minor
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| authorization, | ||
| audience, | ||
| ); | ||
| if (oidcSession) return oidcSession; |
There was a problem hiding this comment.
When a Bearer token is explicitly provided but OIDC validation fails, this code falls through and authenticates via session cookie instead. A request with an invalid JWT should not silently succeed via an alternative auth path — consider returning null here when authorization was present:
| if (oidcSession) return oidcSession; | |
| if (oidcSession) return oidcSession; | |
| // An explicit but invalid bearer token should not fall back to cookie auth. | |
| return null; |
| let jwks: ReturnType<typeof createRemoteJWKSet> | null = null; | ||
|
|
||
| /** @internal Exposed for testing — override to supply a local JWKS resolver. */ | ||
| export function _setJwks(fn: ReturnType<typeof createRemoteJWKSet> | null) { |
There was a problem hiding this comment.
This setter is exported from a production module — any importer can override the JWKS resolver and bypass JWT signature validation. @internal is a docs-only convention in TypeScript; it doesn't restrict imports.
Options:
- Guard with
if (process.env.NODE_ENV !== 'test') throw new Error('not allowed in production'). - Instead of exporting a setter, use
jest.mock('jose', ...)in tests so the production code path is never altered.
| }, | ||
| ); | ||
| if (!loginAcceptResp.ok) { | ||
| throw new Error( |
There was a problem hiding this comment.
Raw Hydra response bodies are included in thrown errors. These can contain sensitive OAuth2 parameters and internal error details that may surface in client responses or log aggregators. Prefer logging the body at LOGGER.error and throwing a sanitised message:
| throw new Error( | |
| const errBody = await loginAcceptResp.text(); | |
| LOGGER.error("Login accept failed", { operation: "getOryIdToken", metadata: { status: loginAcceptResp.status, body: errBody } }); | |
| throw new Error(`Login accept failed: ${loginAcceptResp.status}`); |
| if (!consentChallenge) { | ||
| const location = postLoginResp.headers.get("location"); | ||
| if (location) { | ||
| const resolvedUrl = new URL(location, loginRedirect).toString(); |
There was a problem hiding this comment.
The resolved redirect URL is followed without validating it remains within the expected backendUrl domain. If Hydra returns an unexpected redirect (misconfiguration or compromise), the cookie jar (and potentially auth state) would be sent to an unintended host.
| const resolvedUrl = new URL(location, loginRedirect).toString(); | |
| if (location) { | |
| const resolvedUrl = new URL(location, loginRedirect).toString(); | |
| if (!resolvedUrl.startsWith(backendUrl) && !resolvedUrl.startsWith(redirectUri)) { | |
| throw new Error(`Unexpected redirect to untrusted URL: ${resolvedUrl}`); | |
| } | |
| const followResp = await fetchWithCookies(resolvedUrl, cookieJar, { |
| </Box> | ||
| <Footer /> | ||
| </Flex> | ||
| <ProxyCredentialsProvider isAuthenticated={isAuthenticated}> |
There was a problem hiding this comment.
The ProxyCredentialsProvider is mounted at the top-level app layout, so authenticated users trigger the full Hydra OAuth2 flow (4–6 HTTP requests) on every page — account settings, search, etc. — not just product pages.
Consider moving this provider to the product-specific layout (src/app/(app)/[account_id]/[product_id]/(product)/[[...path]]/layout.tsx) so the credential fetch only runs when the file browser is actually needed.
|
Claude finished @alukach's task in 2m 3s —— View job PR Review: feat/OIDC auth — updated review
This review covers the full current state of the branch, including commits added since the previous review. Several prior concerns remain open; a few new issues were introduced. Still open from previous review1. The fallthrough is unchanged. When an 2. Raw Hydra response bodies in thrown errors
3. Redirect target not validated before following The 4. Still no 5. New issues6. // Before
isDevelopment: process.env.NODE_ENV === "development",
// After
isDevelopment: (process.env.STAGE || "dev") !== "prod",On Vercel preview deployments where 7. Missing redirectUri: `${process.env.NEXT_PUBLIC_SITE_URL || "http://localhost:3000"}/api/internal/oauth2/callback`,
8. The What looks good
|
1 participant
What I'm changing
How I did it
How you can test it