[HYPER-293] fix(pds-core): rewrite sec-fetch-site: same-site on /oauth/authorize - #65
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an E2E security scenario and step definitions exercising PDS handling of Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (client)
participant PDS as PDS Server (Express)
participant Middleware as secFetchSiteRewrite
participant Meta as asMetadataOverride
participant Auth as Auth / PAR endpoint
Browser->>PDS: GET /oauth/authorize?request_uri=...
PDS->>Middleware: evaluate (method, path, sec-fetch-site, referer)
Middleware-->>PDS: (maybe rewrite sec-fetch-site)
PDS->>Meta: apply AS metadata override
PDS->>Auth: POST /oauth/par (to obtain request_uri) / or use provided request_uri
Auth-->>PDS: request_uri (or error)
PDS-->>Browser: response (status + body / redirect)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
🚅 Deployed to the ePDS-pr-65 environment in ePDS
|
Coverage Report for CI Build 24259042703Coverage increased (+0.2%) to 31.705%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/sec-fetch-site.steps.ts`:
- Around line 74-83: The try/catch around res.text() and JSON.parse in
sec-fetch-site.steps.ts currently swallows errors and loses original body text,
and later comparisons against the string 'Forbidden' are case-sensitive; update
the logic in the block that sets this.lastHttpJson (the res.text() and
JSON.parse handling) to always preserve the raw body (e.g., include a property
like bodyRaw or keep body when JSON parsing fails), and ensure any caught errors
are logged at debug level instead of being ignored; additionally change the
'Forbidden' checks to use a case-insensitive comparison (e.g., normalize
toLowerCase()) wherever the code inspects this.lastHttpJson or response text so
sec-fetch-site rejections aren’t missed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22de5a51-76fd-48d2-aecf-6821682f8979
📒 Files selected for processing (1)
e2e/step-definitions/sec-fetch-site.steps.ts
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 15:52
b8ee3de to
a511fc3
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 17:36
a511fc3 to
b301657
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 17:39
b301657 to
cdcfa90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/__tests__/db.test.ts (1)
229-233:⚠️ Potential issue | 🟡 MinorTighten schema-version assertion to cover v9.
Line 233 currently accepts
>= 8, which can pass even if the v9 bump is skipped. This weakens regression detection for this v9-focused block.Suggested test fix
- expect(row.version).toBeGreaterThanOrEqual(8) + expect(row.version).toBeGreaterThanOrEqual(9)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/__tests__/db.test.ts` around lines 229 - 233, The test "schema version is at least 8 after migration" is too weak for the v9 bump; update the assertion in the test (the block using db['db'].prepare('SELECT version FROM schema_version').get()) to require version >= 9 (change expect(row.version).toBeGreaterThanOrEqual(8) to expect(row.version).toBeGreaterThanOrEqual(9)) and also update the test title string to reflect v9 (e.g., "schema version is at least 9 after migration") so the intent matches the assertion.
🧹 Nitpick comments (2)
packages/pds-core/src/index.ts (1)
24-24: Import ordering deviation.Per coding guidelines, imports should be ordered: Node built-ins → external packages → internal workspace packages → local relative imports. This local import is placed before the
node:cryptoimport at line 27.♻️ Suggested reorder
Move the local import after the external packages:
import { applyPdsPortFallback } from './lib/resolve-port.js' -import { shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js' import type * as http from 'node:http' import { randomBytes, timingSafeEqual, createHash } from 'node:crypto' import { PDS, envToCfg, envToSecrets, readEnv } from '@atproto/pds' import { HandleUnavailableError } from '@atproto/oauth-provider' import { generateRandomHandle, createLogger, verifyCallback, escapeHtml, validateLocalPart, resolveClientMetadata, } from '@certified-app/shared' +import { shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` at line 24, The import for the local module shouldRewriteSecFetchSite is out of order; reorder imports to follow Node built-ins → external packages → internal workspace packages → local relative imports by moving the "import { shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js'" line so it appears after the node:crypto import (and after other external imports), ensuring import order conforms to project guidelines.e2e/step-definitions/sec-fetch-site.steps.ts (1)
22-23: UseString.rawto avoid manual escaping.SonarCloud flags that
String.rawshould be used for the regex-like pattern with backslashes.♻️ Suggested fix
When( - 'a GET request is sent to the PDS \\/oauth\\/authorize with sec-fetch-site {string}', + String.raw`a GET request is sent to the PDS /oauth/authorize with sec-fetch-site {string}`, async function (this: EpdsWorld, secFetchSiteValue: string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/sec-fetch-site.steps.ts` around lines 22 - 23, Replace the manually escaped step pattern in the When call with a raw template literal: update the When(...) invocation that currently uses 'a GET request is sent to the PDS \\/oauth\\/authorize with sec-fetch-site {string}' to use String.raw`a GET request is sent to the PDS /oauth/authorize with sec-fetch-site {string}` (i.e., reference the When step definition and change the step string to use String.raw and remove the backslashes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pds-core/src/__tests__/sec-fetch-site-rewrite.test.ts`:
- Around line 1-74: Prettier formatting errors are present in the test file
containing the BASE constant and describe suite for shouldRewriteSecFetchSite;
run your formatter (e.g., run prettier --write on this file) or apply the repo's
Prettier rules to reformat the file so imports, spacing, and trailing commas
match project style, ensuring the BASE declaration, import line, and the
describe/it blocks (including long expect calls) are wrapped and indented per
Prettier output.
---
Outside diff comments:
In `@packages/shared/src/__tests__/db.test.ts`:
- Around line 229-233: The test "schema version is at least 8 after migration"
is too weak for the v9 bump; update the assertion in the test (the block using
db['db'].prepare('SELECT version FROM schema_version').get()) to require version
>= 9 (change expect(row.version).toBeGreaterThanOrEqual(8) to
expect(row.version).toBeGreaterThanOrEqual(9)) and also update the test title
string to reflect v9 (e.g., "schema version is at least 9 after migration") so
the intent matches the assertion.
---
Nitpick comments:
In `@e2e/step-definitions/sec-fetch-site.steps.ts`:
- Around line 22-23: Replace the manually escaped step pattern in the When call
with a raw template literal: update the When(...) invocation that currently uses
'a GET request is sent to the PDS \\/oauth\\/authorize with sec-fetch-site
{string}' to use String.raw`a GET request is sent to the PDS /oauth/authorize
with sec-fetch-site {string}` (i.e., reference the When step definition and
change the step string to use String.raw and remove the backslashes).
In `@packages/pds-core/src/index.ts`:
- Line 24: The import for the local module shouldRewriteSecFetchSite is out of
order; reorder imports to follow Node built-ins → external packages → internal
workspace packages → local relative imports by moving the "import {
shouldRewriteSecFetchSite } from './lib/sec-fetch-site-rewrite.js'" line so it
appears after the node:crypto import (and after other external imports),
ensuring import order conforms to project guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c2f8052-60f0-49dd-9507-de7c851befcd
📒 Files selected for processing (10)
.changeset/consent-upstream-oauth-ui.mdAGENTS.mde2e/cucumber.mjse2e/step-definitions/sec-fetch-site.steps.tsfeatures/security.featurepackages/pds-core/src/__tests__/sec-fetch-site-rewrite.test.tspackages/pds-core/src/index.tspackages/pds-core/src/lib/sec-fetch-site-rewrite.tspackages/shared/src/__tests__/db.test.tspackages/shared/src/db.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/consent-upstream-oauth-ui.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/cucumber.mjs
- features/security.feature
Add a new test to reproduce a bug; it is expected to fail: The upstream @atproto/oauth-provider rejects sec-fetch-site: same-site on GET /oauth/authorize. This breaks ePDS deployments where the auth service and PDS share a registrable domain (e.g. *.certified.app), because the browser sends same-site when following the 303 redirect chain from the auth subdomain to the PDS. CI didn't catch this because Railway's up.railway.app is on the Public Suffix List, making cross-service requests cross-site (which is allowed) rather than same-site. This scenario sends the header directly via HTTP to catch the rejection regardless of CI domain topology. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 17:53
cdcfa90 to
997f262
Compare
aspiers
changed the title
aspiers
changed the title
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:29
a3be915 to
469d6d2
Compare
Stale tsbuildinfo files from the host caused tsc --build to skip compilation inside Docker, producing an empty dist/ directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:37
469d6d2 to
72579d3
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:40
72579d3 to
3e55f34
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:46
3e55f34 to
afa2a83
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:47
afa2a83 to
d2a9099
Compare
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:52
d2a9099 to
b9e32d0
Compare
…horize The upstream @atproto/oauth-provider rejects sec-fetch-site: same-site on GET /oauth/authorize. This breaks ePDS deployments where auth and PDS share a registrable domain (e.g. *.test.certified.app), because browsers send same-site when following the 303 redirect chain from the auth subdomain. Changes: - Add middleware in pds-core to rewrite sec-fetch-site: same-site to same-origin for requests from the trusted auth subdomain - Replace DB migration v9 (DROP client_logins) with a no-op to prevent breaking emergency rollbacks - Update changeset to reflect that client_logins is kept but unused - Add no-drop-tables rule to AGENTS.md - Update db.test.ts to match new migration behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
e2e/sec-fetch-diagnosis
branch
from
April 10, 2026 18:53
b9e32d0 to
313c071
Compare
|
7 tasks
5 tasks
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
sec-fetch-site: same-sitetosame-originforGET /oauth/authorizerequests from the trusted auth subdomainsec-fetch-site: same-sitedirectly via HTTP to catch this regression regardless of CI domain topologyclient_logins) with a no-op to prevent breaking emergency rollbacks**/tsconfig.tsbuildinfoto.dockerignoreto fix stale build cache in DockerFixes HYPER-293
Context
PR #21 changed the epds-callback to redirect through the stock
@atproto/oauth-provider/oauth/authorizeendpoint. On deployments where auth and PDS share a registrable domain (e.g.*.test.certified.app), browsers sendsec-fetch-site: same-siteon the 303 redirect chain from the auth subdomain. The upstream atproto code rejectssame-site(it allowssame-origin,cross-site,none).CI never caught this because Railway's
up.railway.appis on the Public Suffix List, making each Railway subdomain its own "site" — so cross-service requests arecross-site(allowed) rather thansame-site(rejected). Transparent browser-level testing via Playwright'spage.route()is not possible becausesec-fetch-siteis a browser-controlled header that cannot be overridden via CDP, andpage.route()does not intercept redirect chain targets.Test plan
*.test.certified.appdomain🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation