remove toWebRequest and make the package accept nitro event#38
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR expands auth-social to accept flexible "request-like" inputs (framework events, header maps, or Requests), adds normalization helpers, updates exported signatures to accept the new types, adjusts Nuxt routes to pass H3 events directly, adds tests asserting the new input shapes, and expands social-login docs with route and helper examples. ChangesFramework-Agnostic Social Auth Inputs
Sequence DiagramsequenceDiagram
actor Client
participant NuxtRoute as Nuxt Route Handler
participant AuthLib as auth-social Library
participant Normalizer as Request Normalizer
participant OAuth as OAuth Provider
Client->>NuxtRoute: GET /auth/{provider}
NuxtRoute->>AuthLib: redirect(provider, H3Event)
AuthLib->>Normalizer: normalizeSocialRequest(H3Event)
Normalizer->>Normalizer: extract method / url / headers
Normalizer-->>AuthLib: Request
AuthLib->>OAuth: build authorization URL / state
OAuth-->>AuthLib: authorization URL
AuthLib-->>NuxtRoute: HTTP 302 redirect
NuxtRoute-->>Client: Redirect to provider
Client->>NuxtRoute: GET /auth/{provider}/callback?code=...
NuxtRoute->>AuthLib: callback(provider, H3Event)
AuthLib->>Normalizer: normalizeSocialRequest(H3Event)
Normalizer-->>AuthLib: Request
AuthLib->>OAuth: exchange code -> token/profile
OAuth-->>AuthLib: profile & token
AuthLib-->>NuxtRoute: callback result (success/error + local user)
NuxtRoute->>Client: set session / redirect to app
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/auth-social/src/index.ts (2)
157-164: 💤 Low valueConsider documenting the known headers limitation.
When the input header object only provides a
getmethod,appendKnownHeadersonly extracts a hardcoded list of known headers (authorization,cookie,host,x-forwarded-host,x-forwarded-proto). This means any other headers present in the original request won't be included in the normalizedRequest.This is likely acceptable for OAuth flows (which primarily need these headers), but documenting this limitation would help future maintainers understand why this approach was chosen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-social/src/index.ts` around lines 157 - 164, The function appendKnownHeaders only copies a hardcoded set of header names (authorization, cookie, host, x-forwarded-host, x-forwarded-proto) from an input object that exposes only a get method, so other headers on the original request are intentionally not propagated; update documentation/comments near the appendKnownHeaders function (and its signature using Headers and input.get) to explicitly state this limitation, why it's acceptable for OAuth flows, and note that additional headers will be ignored unless the implementation is changed to accept a full header map or iterator.
243-257: 💤 Low valueConsider security implications of URL reconstruction fallbacks.
When the URL cannot be parsed directly, the code reconstructs it from headers with fallbacks to
'http'protocol and'localhost'host. In production environments behind proxies, missingx-forwarded-protoandx-forwarded-hostheaders could result in incorrect URLs being constructed.While these fallbacks are reasonable for development, consider whether it's safer to throw an error when critical headers are missing in production scenarios, rather than silently falling back to potentially incorrect values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-social/src/index.ts` around lines 243 - 257, getRequestLikeUrl currently falls back to 'http' and 'localhost' when headers are missing; update it to validate critical forwarded headers in production: inside getRequestLikeUrl (and where it reads headers.get('x-forwarded-proto') and headers.get('x-forwarded-host')), detect production mode (e.g. process.env.NODE_ENV === 'production' or a new config flag like requireForwardedHeaders) and if in production and either header is missing, throw a clear error instead of silently defaulting; preserve the existing header fallbacks only for non-production/dev mode and update any tests that assume the old default behavior to cover both dev fallback and production error paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/auth-social/src/index.ts`:
- Around line 157-164: The function appendKnownHeaders only copies a hardcoded
set of header names (authorization, cookie, host, x-forwarded-host,
x-forwarded-proto) from an input object that exposes only a get method, so other
headers on the original request are intentionally not propagated; update
documentation/comments near the appendKnownHeaders function (and its signature
using Headers and input.get) to explicitly state this limitation, why it's
acceptable for OAuth flows, and note that additional headers will be ignored
unless the implementation is changed to accept a full header map or iterator.
- Around line 243-257: getRequestLikeUrl currently falls back to 'http' and
'localhost' when headers are missing; update it to validate critical forwarded
headers in production: inside getRequestLikeUrl (and where it reads
headers.get('x-forwarded-proto') and headers.get('x-forwarded-host')), detect
production mode (e.g. process.env.NODE_ENV === 'production' or a new config flag
like requireForwardedHeaders) and if in production and either header is missing,
throw a clear error instead of silently defaulting; preserve the existing header
fallbacks only for non-production/dev mode and update any tests that assume the
old default behavior to cover both dev fallback and production error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c4eb67a-c743-4ae6-9aa2-3a1b4013abca
📒 Files selected for processing (8)
apps/blog-nuxt/server/lib/request.tsapps/blog-nuxt/server/routes/auth/github.get.tsapps/blog-nuxt/server/routes/auth/github/callback.get.tsapps/blog-nuxt/server/routes/auth/google.get.tsapps/blog-nuxt/server/routes/auth/google/callback.get.tsapps/docs/docs/auth/social-login.mdpackages/auth-social/src/index.tspackages/auth-social/tests/package.test.ts
💤 Files with no reviewable changes (1)
- apps/blog-nuxt/server/lib/request.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests