Add integration test: OAuth Resource path-segment routing#102
Merged
Conversation
Regression guard for the symptom Dawson reported on 2026-05-22 (CM/Studio
Okta SSO): "providerName is 'oauth', not the {configId} described, that
isn't passed".
The fixture configures two static providers — one literally named "oauth"
(a decoy) and one named "oac-tenant-acme" with a distinctive client_id.
The test fires /oauth/oac-tenant-acme/login and asserts the redirect goes
to the tenant provider, not the decoy.
If parseRoute ever regresses into extracting the literal "oauth" path
prefix as providerName (the reported symptom), the decoy provider would
incorrectly handle the request and the assertion would fail.
This test PASSES on main + Harper 5.0.7, which means the bug as
diagnosed is not reproducible in this code path on this stack. The
v1.x / Harper v4 stack is not covered by this test; CM consumes
@harperfast/oauth ^1.4.0 against harperdb ^4.7.28, and that combination
may or may not exhibit the same routing behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewed; no blockers found. |
Contributor
|
Reviewed; no blockers found. |
3 tasks
Two review-driven adjustments to the path-segment-routing fixture: 1. Tighten the docstring/comment wording about what a parseRoute regression would look like in this test. Codex caught that "the decoy would handle the request" overstates the failure mode — with the bug, action would be the tenant URL segment (an unknown action → 404), so the test would fail on `expected 302` rather than redirecting to decoy.test. Reworded both files to describe the failure as "would NOT produce a tenant-shaped 302" and lean on the assertions to surface the specific mismatch. 2. Rename the tenant provider from oac-tenant-acme to oac-oauth-tenant (agy's suggestion). The substring "oauth" inside the providerName now also guards against an over-aggressive stripping regression — e.g., a future parseRoute that mangles /oauth/ matches occurring inside the segment, not just at the prefix. Renamed the env var value and the URL accordingly. Also split the url.origin/pathname assertion to avoid string-concat false-failure modes (agy suggestion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
heskew
added a commit
that referenced
this pull request
May 26, 2026
…uild Acts on the agy + codex review pass on this PR. Fixes (blocker + significant): 1. **Backup-overwrite race (blocker, agy)** — relocate the boot-props backup from a hardcoded /tmp path into the per-run tempRoot. On an unclean rerun, the previous design copied the now-dirty BOOT_PROPS_PATH over the still-original backup at the hardcoded path, permanently losing the user's real ~/.harperdb config. With the backup under tempRoot (mkdtemp'd per run), prior leftover backups can't be overwritten — and an existsSync check inside backupBootProps provides a belt-and-suspenders guard if tempRoot were ever reused. 2. **Cleanup path unification (significant, codex)** — replace the process.exit() short-circuit in the signal handlers with a single synchronous `cleanup()` function that the finally block AND every signal handler call. cleanup() now: kills harperdb (with async SIGKILL fallback), restores boot props via the same code path used by normal exits, and removes tempRoot. waitForReady also subscribes to the child process's `error` event so ENOENT / EACCES route through the same cleanup. Adds SIGHUP / SIGQUIT in addition to SIGINT / SIGTERM. 3. **/tmp multi-user collision (significant, agy)** — covered by #1 (backup is now inside the run-scoped tempRoot, no shared-tmp races). 4. **node_modules cp waste (significant, agy)** — eliminate the fixtureDir → components/app recursive copy. Write config.yaml / package.json directly into tempRoot/components/app and run npm install there. Saves seconds of recursive-copy per run. 5. **Silent stale dist/ (significant, codex)** — replace `npm run build` (which is `tsc || true`) with: rm dist/, invoke the local tsc binary directly, then assert dist/index.js was emitted. A TS emission failure now stops the script instead of letting it pass against the previous build. 6. **Peer-dep registry drift (significant, codex)** — pin the fixture's harperdb peer to the exact version (4.7.19) being spawned by the script. Removes the risk that `npm install` resolves a different registry version than what the script's harperBin is. Also adopts agy's `oac-oauth-tenant` rename: the tenant provider name now contains the substring "oauth" so this script (and its v5 counterpart in PR #102) also catches an over-aggressive stripping regression in addition to the prefix-extraction case. Out of scope: the v5 layer applied to v1.x AI reviews (both reviewers flagged this; needs a branch-aware layer input in HarperFast/ai-review-prompts, not changes here). Verified locally: run produces PASS, ~/.harperdb/hdb_boot_properties.file restored to its pre-run state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 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
Adds an integration test that asserts the OAuth Resource correctly routes by URL path segment (the provider name from the URL) and not by the literal
oauthprefix.Originally motivated by Dawson's report from 2026-05-22 (CM/Studio Okta SSO):
The new test configures two static providers — one literally named
"oauth"(a decoy) and one named"oac-tenant-acme"with a distinctiveclient_id— then firesGET /oauth/oac-tenant-acme/loginand asserts the redirect went to the tenant provider'sauthorizationUrlwith the tenant'sclient_id. IfparseRouteever regressed into Dawson's described behavior, the decoy provider would handle the request and the assertion would fail.Investigation result
The test PASSES on
main+ Harper 5.0.7. When I temporarily loggedtarget.idinsideparseRoute, Harper v5 was passing"oac-fake-12345/login"(i.e. the/oauth/resource mount prefix already stripped). So on this stack,providerNameis correctly the URL path segment — Dawson's diagnosis is not reproducible in this code path.That doesn't rule the bug out for Dawson's stack:
central-manager,main) consumes@harperfast/oauth ^1.4.0againstharperdb ^4.7.28.parseRouteis byte-for-byte identical between v1.4.0 andmain, but Harper v4 may surfacetarget.iddifferently than v5. Porting this test to thev1.xline would require bootstrapping@harperfast/integration-testingagainstharperdb(its peer dep isharper ^5.0.0), so we deferred that for now.src/index.ts:226calls the resolver withrequest.session.oauth.providerConfigId— session data, not URL-derived. A stale session containingproviderConfigId === 'oauth'would invoke CM's resolver with'oauth'and produce the exact symptom Dawson observed, independent of any routing bug.Failed to resolve OAuth provider(resource.ts:343-346) only fires when the hook throws; a hook returningnullproduces a 404. There's a separate exception path on the CM side worth tracking down.Why no fix in this PR
oauth#101proposes stripping a leadingoauthsegment inparseRoutedefensively. Given this test passes without that change, the patch is unnecessary onmainand would actively break a provider literally namedoauth(which is the decoy this test now exercises).Test plan
npm run install:fixtures && npm run test:integration— both suites pass (env-var + new path-segment-routing).v1.x+ Harper v4.providerConfigId === 'oauth'to confirm the session-middleware hypothesis.🤖 Generated with Claude Code