feat: Auth always enable in all environments - #102
Merged
Conversation
… OIDC client settings Co-authored-by: Copilot <copilot@github.com>
…gets, enhancing auth handling and security settings
Co-authored-by: Copilot <copilot@github.com>
…emove legacy auth handling Co-authored-by: Copilot <copilot@github.com>
…streamline Keycloak integration
…emove legacy header support
Contributor
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAuthentication becomes always-on and build-target driven: runtime Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Proxy as proxy.ts
participant App as Next.js server (/api/auth/login, /api/auth/callback, /api/auth/me)
participant IdP as Keycloak (OIDC)
Browser->>Proxy: GET /api/auth/login
Proxy->>App: forward /api/auth/login (headers stripped)
App->>IdP: Redirect to Keycloak (auth request, PKCE/state)
Browser->>IdP: Follow redirect, authenticate (user submits credentials)
IdP->>Browser: Redirect to /api/auth/callback with code
Browser->>Proxy: GET /api/auth/callback (headers stripped)
Proxy->>App: forward /api/auth/callback
App->>IdP: Exchange code for tokens, validate issuer/audience
App->>App: Create iron-session cookie, attach verified actor
App->>Browser: 302 redirect to app with session cookie set
Browser->>Proxy: GET /api/auth/me
Proxy->>App: forward /api/auth/me
App->>Browser: return authenticated user JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
- Coverage 57.35% 57.31% -0.04%
==========================================
Files 257 260 +3
Lines 17013 16967 -46
Branches 6441 6519 +78
==========================================
- Hits 9757 9725 -32
+ Misses 7143 7129 -14
Partials 113 113
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/requirements/auth.ts (1)
31-37:⚠️ Potential issue | 🟡 MinorStale doc comment references the removed header-trust path.
The JSDoc on
hsaIdstill says it'snullfor "header-trust" actors, but the header-derived path was removed in this PR (the'headers'value was dropped fromActorSourceon Line 25). Please drop that mention so the doc matches the new identity model.📝 Suggested doc tweak
/** * HSA-id when the actor was authenticated and a verified `employeeHsaId` - * claim was present. `null` for anonymous, header-trust, or MCP actors - * whose token did not carry the claim. MCP service-account tokens carry - * the synthetic `mcp-client:<client_id>` value here. + * claim was present. `null` for anonymous actors and for MCP actors whose + * token did not carry the claim. MCP service-account tokens carry the + * synthetic `mcp-client:<client_id>` value here. */ hsaId: string | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/requirements/auth.ts` around lines 31 - 37, The JSDoc for hsaId incorrectly mentions "header-trust" actors which no longer exist after removing the 'headers' variant from ActorSource; update the comment on the hsaId field in lib/requirements/auth.ts to remove the "header-trust" reference so it matches the new identity model (keep notes about null for anonymous or MCP actors and the synthetic `mcp-client:<client_id>` value for MCP service-account tokens).
🧹 Nitpick comments (15)
dev/keycloak/realm-kravhantering-dev.json (2)
129-179: Update auth developer workflow docs for the newkravhantering-localclient.A new realm client (
kravhantering-local, port 3001, secretlocal-kc-app-secret) was added for the prodlike target, butdocs/auth-developer-workflow.md(and/ordocs/auth-how-it-works.md) doesn't appear to mention it. Developers running the local-prod / prodlike flow against this realm need to know which client/secret to point at.Based on learnings: "Update
docs/auth-developer-workflow.mdwhenever a change affects local Keycloak setup".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/keycloak/realm-kravhantering-dev.json` around lines 129 - 179, The docs lack mention of the new Keycloak client "kravhantering-local" (clientId: kravhantering-local, secret: local-kc-app-secret, port: 3001) added for the prodlike/local-prod flow; update docs/auth-developer-workflow.md (and optionally docs/auth-how-it-works.md) to document this client: where to configure redirect URIs (http://localhost:3001/api/auth/callback and http://127.0.0.1:3001/api/auth/callback), webOrigins, the client secret, and that this client is used for the prodlike target when running locally so developers know which clientId/secret and port to point their local app at.
129-179: Consider hardening the prod-bundle verifier againstkravhantering-local/local-kc-app-secretleaks.
scripts/verify-prod-bundle.mjsalready listskravhantering-appanddev-onlyas forbidden tokens to catch dev artifacts leaking into the prod bundle. The new local-prod-only client id (kravhantering-local) and its secret (local-kc-app-secret) won't match either pattern. If any code path ever statically embeds these (e.g., a fallback constant), the verifier won't catch it. Adding them toFORBIDDENkeeps the contract tight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/keycloak/realm-kravhantering-dev.json` around lines 129 - 179, The prod-bundle verifier doesn't currently block the new local client id/secret from the Keycloak JSON; update the FORBIDDEN list in scripts/verify-prod-bundle.mjs to include "kravhantering-local" and "local-kc-app-secret" so the verifier will catch any accidental static embedding of those dev-only values; ensure the additions are added to the same FORBIDDEN constant/array that already contains "kravhantering-app" and "dev-only" and run the verifier tests.lib/runtime/build-target.prod.ts (1)
16-26: Optional: use literal types for stronger compile-time DCE narrowing.Annotating these as
: booleanwidens away the literalfalse, so TypeScript itself cannot narrowif (USE_DEV_CSP) { ... }toneverin consumers. Webpack constant-folding still eliminates the dead branches at bundle time (which is whatverify-prod-bundlechecks), but stronger TS narrowing would catch dead-code-elimination regressions during type-checking, not just at build-verify time.📝 Suggested narrowing
-export const BUILD_TARGET: BuildTarget = 'prod' +export const BUILD_TARGET = 'prod' as const satisfies BuildTarget @@ -export const ALLOW_INSECURE_OIDC_ISSUER: boolean = false +export const ALLOW_INSECURE_OIDC_ISSUER = false as const @@ -export const USE_INSECURE_COOKIE: boolean = false +export const USE_INSECURE_COOKIE = false as const @@ -export const USE_DEV_CSP: boolean = false +export const USE_DEV_CSP = false as constNote: this would need to be applied symmetrically to the other build-target files so the alias-swap module shape stays compatible across targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/runtime/build-target.prod.ts` around lines 16 - 26, The constants ALLOW_INSECURE_OIDC_ISSUER, USE_INSECURE_COOKIE, and USE_DEV_CSP are annotated as boolean which widens them away from the literal false; change their types to the literal false (e.g., export const USE_DEV_CSP: false = false) or use const assertions so TypeScript narrows branches at compile time (leave BUILD_TARGET as 'prod'); apply the same literal-typing pattern symmetrically across the other build-target files so the module shape remains compatible..github/workflows/quality-checks.yml (1)
106-107: Thenpm run dotenv:checkscript exists and does cover both paths, but has different exclusions than the workflow actions.The npm script (
dotenv:checkin package.json) correctly runs dotenv-linter on both root (.) and.devcontainer --recursive. However, it applies specific exclusions (.env.sqlserver,.env.local,.env.*.localat root;.devcontainer/.env,.devcontainer/elevated/.envin devcontainer) that are not explicitly configured in the workflow actions, meaning running locally may not reproduce the exact same linting behavior as the workflow.Not introduced by this PR—flagging only because the new entry was added next to it. Consider aligning exclusion patterns between the npm script and the workflow actions to ensure consistency between local development checks and CI/CD validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/quality-checks.yml around lines 106 - 107, The workflow's dotenv-linter action (entry named 'dotenv-linter' running "npm run dotenv:check") and the package.json script 'dotenv:check' use different exclusion patterns so CI and local runs can diverge; update either the workflow action or the 'dotenv:check' npm script so they use the exact same dotenv-linter exclude globs (e.g., root exclusions like .env.sqlserver, .env.local, .env.*.local and devcontainer exclusions like .devcontainer/.env and .devcontainer/elevated/.env) — locate the 'dotenv:check' script in package.json and the 'dotenv-linter' entry in the workflow (name: 'dotenv-linter') and make their exclusion flags identical to ensure parity between local and CI linting.tests/unit/mcp-token.test.ts (1)
83-197: Inconsistent cleanup: strayenabled: trueleft in othergetAuthConfigmocks.The first test (Line 31-42) was correctly trimmed to drop
enabledfrom the mockedAuthConfig, matching the new contract whereenabledno longer exists. However, the remaining tests in this file still setenabled: trueon the mock return values (Lines 85, 108, 134, 164, 193). These references are now meaningless and easy to misread as if the auth-enabled flag still has effect. Consider removing them for consistency.♻️ Proposed cleanup
it('accepts a real HSA-id in employeeHsaId', async () => { getAuthConfigMock.mockReturnValue({ - enabled: true, issuerUrl: 'https://issuer.example.com', apiAudience: 'kravhantering-app', })(Apply the same removal at Lines 108, 134, 164, and 193.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-token.test.ts` around lines 83 - 197, The tests still include the obsolete enabled property in getAuthConfigMock.mockReturnValue calls, causing inconsistent mocks; remove enabled: true from every getAuthConfigMock.mockReturnValue usage in this test file (used in the blocks that set issuerUrl/apiAudience in the verifyMcpBearerToken tests and the security audit events beforeEach) so the mocked AuthConfig matches the new contract—look for getAuthConfigMock.mockReturnValue({...}) in the tests around the verifyMcpBearerToken specs and the beforeEach and delete the enabled: true entries.vitest.config.ts (1)
71-90: Move the@/lib/runtime/build-targetalias before the@alias to ensure more specific rules are matched first.Vite evaluates aliases in declaration order (first match wins). The
@alias at line 74 matches any import starting with@/(including@/lib/runtime/build-target), so the more specific alias below it is never consulted. Today both resolve to the same file, but if the explicit alias is later pointed to a different file (e.g., a test stub or dev variant), the swap will silently fail, defeating the stated intent.♻️ Proposed reorder
resolve: { alias: { - // Module path mapping (equivalent to Jest's moduleNameMapper) - '@': path.resolve(__dirname, '.'), // Always resolve the build-target to the dev implementation in tests. // Webpack aliases do not apply to vitest; this explicit alias ensures // tests never accidentally use the local-prod or prod frozen constants. '@/lib/runtime/build-target': path.resolve( __dirname, 'lib/runtime/build-target.ts', ), + // Module path mapping (equivalent to Jest's moduleNameMapper) + '@': path.resolve(__dirname, '.'), '@viscalyx/developer-mode-core': path.resolve(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 71 - 90, The alias ordering causes the general '@' alias to shadow the more specific '@/lib/runtime/build-target' rule; move the '@/lib/runtime/build-target' entry so it appears before the '@' alias in the resolve.alias object in vitest.config.ts so the specific alias is matched first (ensure the keys '@/lib/runtime/build-target' and '@' are reordered accordingly).tests/unit/auth-me-route.test.ts (1)
7-34: Optional cleanup: Remove unusedgetAuthConfigmock from test setup.The
app/api/auth/me/route.tshandler imports and uses onlygetSession()andisSignedIn(). It never calls or importsgetAuthConfig(). Remove the unused mock definition, thevi.mock('@/lib/auth/config', ...)block, and thegetAuthConfigMock.mockReturnValue({})calls from both test cases to keep the test file aligned with the route's actual dependencies.♻️ Proposed cleanup
import { beforeEach, describe, expect, it, vi } from 'vitest' -const getAuthConfigMock = vi.fn() const getSessionMock = vi.fn() const isSignedInMock = vi.fn() -vi.mock('@/lib/auth/config', () => ({ - getAuthConfig: () => getAuthConfigMock(), -})) - vi.mock('@/lib/auth/session', () => ({ getSession: () => getSessionMock(), isSignedIn: (...args: unknown[]) => isSignedInMock(...args), })) import { GET } from '@/app/api/auth/me/route' describe('auth me route', () => { beforeEach(() => { vi.clearAllMocks() }) it('returns unauthenticated responses with no-store caching', async () => { - getAuthConfigMock.mockReturnValue({}) getSessionMock.mockResolvedValue({}) isSignedInMock.mockReturnValue(false) @@ it('returns authenticated responses with no-store caching', async () => { - getAuthConfigMock.mockReturnValue({}) getSessionMock.mockResolvedValue({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth-me-route.test.ts` around lines 7 - 34, Remove the unused getAuthConfig mock and its uses: delete the vi.mock('@/lib/auth/config', ...) block and remove any getAuthConfigMock.mockReturnValue({}) calls in the tests for the GET handler; keep only the mocks for getSession and isSignedIn (getSessionMock, isSignedInMock) which are actually used by the GET function in app/api/auth/me/route. Ensure tests still call GET() and assert headers/body unchanged after removing getAuthConfig-related setup.playwright.config.ts (1)
62-73: Normalize theOriginheader derived fromPLAYWRIGHT_BASE_URL.
Originis built by reusingPLAYWRIGHT_BASE_URLverbatim. If a user setsPLAYWRIGHT_BASE_URL=http://localhost:3000/(trailing slash) or with a path, theOriginheader value becomes non-canonical and the same-origin check inlib/auth/csrf.ts(which compares URL origins) will start rejecting mutating requests in their local Playwright runs. Fine for the default value, but a footgun the moment someone overrides the env var.🛡️ Suggested hardening
- baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:3000', + baseURL: new URL( + process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:3000', + ).origin, @@ - extraHTTPHeaders: { - Origin: process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:3000', - 'X-Requested-With': 'XMLHttpRequest', - }, + extraHTTPHeaders: { + Origin: new URL( + process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:3000', + ).origin, + 'X-Requested-With': 'XMLHttpRequest', + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 62 - 73, Normalize the Origin header by deriving it from the env var using URL parsing instead of reusing PLAYWRIGHT_BASE_URL verbatim: compute an origin value via new URL(process.env.PLAYWRIGHT_BASE_URL).origin (with a try/catch and fallback to 'http://localhost:3000') and use that for extraHTTPHeaders. Update the extraHTTPHeaders Origin assignment (and keep baseURL as is) so Origin is the canonical origin string; reference extraHTTPHeaders, PLAYWRIGHT_BASE_URL, and baseURL and ensure this matches the same-origin checks in lib/auth/csrf.ts.tests/unit/build-target.test.ts (1)
67-74: Consider broadening the “noprocess.env” check.The regex
/process\.env\.[A-Z_]/only catches dotted access. A future regression that uses bracket notation (process.env['ALLOW_INSECURE_OIDC_ISSUER']) or aliases theprocess.envobject first would slip past this guard, while still violating the “noprocess.envreads” contract documented at the top oflib/runtime/build-target.prod.ts.♻️ Tighter assertion
- // Match actual property accesses like process.env.FOO, not prose mentions. - expect(src).not.toMatch(/process\.env\.[A-Z_]/) + // Match actual reads of process.env in any access form, not prose mentions. + // Strip line-comments and block-comments first so doc-text mentions don't + // trip the assertion. + const code = src + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/.*$/gm, '$1') + expect(code).not.toMatch(/process\s*\.\s*env\s*\.\s*[A-Z_]/) + expect(code).not.toMatch(/process\s*\.\s*env\s*\[/) + expect(code).not.toMatch(/process\s*\[\s*['"]env['"]\s*\]/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/build-target.test.ts` around lines 67 - 74, The test "contains no process.env reads" currently only detects dotted access and misses bracket notation and aliases; update the assertion in tests/unit/build-target.test.ts to use a broader pattern that matches process.env followed by either a dot property access, a bracketed string property access, or any bare "process.env" occurrence (which would indicate aliasing/destructuring), and assert that the source of lib/runtime/build-target.prod.ts does not match that broader pattern so bracket notation and aliasing regressions are caught.scripts/verify-prod-bundle.mjs (2)
41-115: Consider extracting the prod-export contract check for unit testing.The Part 1 logic (extras / missing / value-mismatch comparison against
EXPECTED_PROD_EXPORTS) is pure and is exactly the kind of "deterministic script logic" the repository guideline asks to keep covered. Pulling the comparison into an exported helper (e.g.compareProdExports(actual, expected)returning{ extras, missing, mismatched }) would make it trivial to assert its three failure modes from Vitest without spawning Node or touching the filesystem.As per coding guidelines: "Treat deterministic script logic as production code and keep changed files at >= 85% coverage" and "Prefer extracting pure functions from CLI wrappers so behavior is easy to test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-prod-bundle.mjs` around lines 41 - 115, The verification logic in verifyBuildTargetProd is pure and should be extracted into a testable helper; create and export a function (e.g. compareProdExports(actualExports, expected = EXPECTED_PROD_EXPORTS)) that returns an object like { extras, missing, mismatched } where mismatched lists keys with differing values, then refactor verifyBuildTargetProd to call this helper and use its results to set failed/log messages; keep the existing EXPECTED_PROD_EXPORTS constant and ensure the new function is exported so Vitest unit tests can assert the three failure modes without filesystem or Node import side effects.
152-175: Harden the grep invocation: use fixed-string mode and avoid shell quoting.
JSON.stringify(token)only handles JSON escaping; the resulting double-quoted bash word still expands$, backtick, and\, so adding any future token containing those would silently misbehave. Likewise,grepwithout-Finterprets the token as a basic regex — fine for the current alphanumeric-and-hyphen tokens but brittle long term. Switching tospawnSyncwith array args plusgrep -F -rremoves both footguns at once.🛡️ Suggested change
-import { execSync } from 'node:child_process' +import { spawnSync } from 'node:child_process' @@ - for (const token of FORBIDDEN) { - for (const dir of [SERVER_DIR, STATIC_DIR]) { - let output = '' - try { - output = execSync( - `grep -r --include="*.js" -l ${JSON.stringify(token)} ${JSON.stringify(dir)}`, - { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] }, - ).trim() - } catch { - // grep exits 1 when nothing is found — that is the success case here. - output = '' - } - - if (output.length > 0) { + for (const token of FORBIDDEN) { + for (const dir of [SERVER_DIR, STATIC_DIR]) { + const result = spawnSync( + 'grep', + ['-rFl', '--include=*.js', '--', token, dir], + { encoding: 'utf8' }, + ) + // grep exits 1 when nothing is found — that is the success case here. + // Anything other than 0 or 1 is a real failure. + if (result.status !== 0 && result.status !== 1) { + console.error( + `[verify-prod-bundle] grep failed (exit ${result.status}): ${result.stderr}`, + ) + failed = true + continue + } + const output = (result.stdout ?? '').trim() + if (output.length > 0) { console.error( `[verify-prod-bundle] FORBIDDEN token "${token}" found in bundle:`, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-prod-bundle.mjs` around lines 152 - 175, The grep invocation is brittle and uses shell quoting via JSON.stringify and regex mode; replace execSync with child_process.spawnSync to call grep directly and pass args as an array including '-F' and '-r' and '--include=*.js' plus the token as a separate argument, iterating over FORBIDDEN and [SERVER_DIR, STATIC_DIR] as before; treat exit status 1 as "not found" (no output) and on exit code 0 read stdout (use encoding: 'utf8') to populate output, preserving the same logging and setting of failed when output is non-empty so token hits are reported.playwright.prodlike.config.ts (1)
29-53: LGTM — webServer and storageState now consistently target the always-auth prodlike posture.Switching to
start:prodlikeand pinningstorageStatetotest-results/auth/admin.jsoncorrectly aligns with the always-on auth contract. The new comment block makes the dev-IdP prerequisite explicit.One nit (defer-able): if a contributor runs with
PLAYWRIGHT_SKIP_WEBSERVERandglobalSetuphasn't populatedtest-results/auth/admin.json, every spec fails with an opaque ENOENT. Consider adding a guard intests/integration/global-setup.ts(out of scope here) that surfaces a clearer "runnpm run idp:upand rerun" message — verifying that's already covered would close the loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.prodlike.config.ts` around lines 29 - 53, Add a guard in the global setup to check for the pinned storage state file and surface a clear error when it's missing: in tests/integration/global-setup.ts (the globalSetup function), detect when process.env.PLAYWRIGHT_SKIP_WEBSERVER is truthy and verify the file 'test-results/auth/admin.json' exists before proceeding; if the file is not found, throw or log an explicit error that tells the contributor to run the dev IdP (e.g., "run `npm run idp:up` and re-run tests") so specs don't fail with an opaque ENOENT.docs/auth-how-it-works.md (1)
28-31: Minor sentence-flow nit on Line 28.The sentence reads
proxy.ts is the front door. Auth is always on, so it: allows public paths, redirects .... The colon mid-sentence after "so it:" inside running prose is awkward — consider a small reflow such as "Auth is always on, so the proxy allows public paths, redirects unauthenticated browser page requests to/api/auth/login, returns401for unauthenticated API requests, and requires a Bearer header for/api/mcp." Purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/auth-how-it-works.md` around lines 28 - 31, Reflow the sentence in docs/auth-how-it-works.md to remove the awkward mid-sentence colon and make it run smoothly; replace the two-sentence/colon form that starts with "proxy.ts is the front door. Auth is always on, so it:" with a single sentence like "Auth is always on, so the proxy (`proxy.ts`) allows public paths, redirects unauthenticated browser page requests to `/api/auth/login`, returns `401` for unauthenticated API requests, and requires a Bearer header for `/api/mcp`." Ensure the reference to proxy.ts and the endpoint paths remain accurate.lib/requirements/auth.ts (1)
175-208: Confirm the intent of callinggetAuthConfig()for its side effect.
getAuthConfig()is invoked purely to assert config is present (Line 183), then its return value is discarded. After the first request the result is cached, so subsequent calls are cheap, but the pattern is subtle — a future reader could easily delete the line as "dead code". Consider either capturing/using the result or extracting a named helper (e.g.assertAuthConfigLoaded()) so the intent is unmistakable. Functionally fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/requirements/auth.ts` around lines 175 - 208, The call to getAuthConfig() in getActorContextFromSession is currently only for its side effect (ensuring auth env vars) and looks like dead code; make the intent explicit by replacing the bare call with either (a) capture and use its result (e.g., const authConfig = getAuthConfig(); /* use authConfig or pass to helper */) or (b) extract a clearly named helper like assertAuthConfigLoaded() and call that from getActorContextFromSession so readers know the call is intentional and won’t remove it; update the code paths that rely on auth config accordingly (references: getActorContextFromSession, getAuthConfig, consider adding assertAuthConfigLoaded).package.json (1)
20-20: Redundant outerdotenv -e .env.prodlike --wrapper beforenpm run build:local-prod.
build:local-prodalready wrapsnext buildwithdotenv -e .env.prodlike -- cross-env ..., so the outerdotenvon the build half ofstart:prodlikeis duplicative and just adds startup overhead. The seconddotenv(after&&) is still needed fornext start.♻️ Proposed simplification
- "start:prodlike": "dotenv -e .env.prodlike -- npm run build:local-prod && dotenv -e .env.prodlike -- cross-env NODE_ENV=production BUILD_TARGET=local-prod next start --hostname 0.0.0.0 --port 3001", + "start:prodlike": "npm run build:local-prod && dotenv -e .env.prodlike -- cross-env NODE_ENV=production BUILD_TARGET=local-prod next start --hostname 0.0.0.0 --port 3001",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 20, The start:prodlike npm script redundantly prepends dotenv -e .env.prodlike -- to npm run build:local-prod even though build:local-prod already applies dotenv; remove the outer dotenv wrapper so start:prodlike simply runs npm run build:local-prod && dotenv -e .env.prodlike -- cross-env NODE_ENV=production BUILD_TARGET=local-prod next start --hostname 0.0.0.0 --port 3001, leaving the second dotenv before next start intact; update the "start:prodlike" entry in package.json and verify the script name "build:local-prod" remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/quality-checks.yml:
- Around line 63-65: Add an explicit prod build step before the "Verify prod
build-target contract" step so that the verify:prod-bundle script can run its
second security check against actual build artifacts; specifically, insert a
step that runs "npm run build" (or your production build script) prior to
invoking the npm script "verify:prod-bundle" referenced in the step named
"Verify prod build-target contract" so that the grepping of .next/server and
.next/static is not skipped.
In `@app/api/auth/logout/route.ts`:
- Around line 32-35: The GET handler currently just returns
createLocalRedirect(request, cfg.postLogoutRedirectUri) and must also clear the
user's session to avoid leaving a valid cookie; modify the exported GET function
to call await getSession(request) (or the existing session getter used by the
POST flow), await session.destroy() when a session exists, then proceed to
createLocalRedirect with cfg.postLogoutRedirectUri; reference the GET handler,
getAuthConfig, getSession/session.destroy, and createLocalRedirect so reviewers
can find and mirror the POST handler's logout steps.
In `@docs/arkitekturbeskrivning-kravhantering.md`:
- Around line 1198-1201: Update the misleading doc wording to reference the
actual compile-time exported constant ALLOW_INSECURE_OIDC_ISSUER (from
`@/lib/runtime/build-target`) instead of the old env-var name
AUTH_OIDC_ALLOW_INSECURE_ISSUER; change the fail-closed bullet in
docs/arkitekturbeskrivning-kravhantering.md to state that
ALLOW_INSECURE_OIDC_ISSUER is a build-target compile-time constant (true only in
dev/local-prod) and no longer an environment variable, and make the same
consistent replacement and wording check in docs/auth-how-it-works.md so both
docs refer to the exported symbol and the compile-time semantics.
In `@docs/auth-developer-workflow.md`:
- Around line 9-11: The wording omits the local-prod build target: update the
sentence that mentions "Keycloak in dev, the real OIDC provider in deployed
environments" to explicitly include the local-prod target (which also uses the
local Keycloak at http://localhost:8080) so readers don't assume start:prodlike
uses a real provider; reference the build target name "local-prod" and the env
var "AUTH_OIDC_ALLOW_INSECURE_ISSUER" in the revised sentence so it’s clear
local-prod points to the local Keycloak and that insecure issuer usage is
configurable.
In `@next.config.ts`:
- Around line 11-26: The code lets an empty-string BUILD_TARGET slip through;
change how buildTarget is derived and validated so empty or whitespace-only
values are treated as unset: read the raw env (e.g., rawTarget =
process.env.BUILD_TARGET), normalize with trim() and treat '' as undefined, then
compute buildTarget = normalized ?? (isProduction ? undefined : 'dev'); update
the subsequent checks that currently use truthiness (the production guard and
the whitelist guard) to explicitly test for undefined (e.g., if (isProduction &&
buildTarget === undefined) and if (buildTarget !== undefined &&
!['dev','local-prod','prod'].includes(buildTarget))) so an empty BUILD_TARGET
triggers the Unknown BUILD_TARGET error and resolvedBuildTarget logic no longer
receives a blank string.
In `@package.json`:
- Line 23: The "check" npm script currently runs "verify:prod-bundle" which
silently skips the bundle grep when .next/server and .next/static are absent,
causing false positives; update either the package.json "check" pipeline to
ensure a prod build is present (prepend or guarantee "npm run build" before
"verify:prod-bundle") or modify scripts/verify-prod-bundle.mjs to assert
presence of .next/server and .next/static (or fail after Part 1 /
build-target.prod.ts validation) so the process exits nonzero when artifacts are
missing; reference the "check" script, verify-prod-bundle.mjs, and
build-target.prod.ts when applying the change.
In `@tests/integration/global-setup.ts`:
- Around line 117-138: The success log inside globalSetup that reports a stored
session for each spec is using console.warn; change that one to console.info (or
console.log) so only the skip branch uses console.warn. Locate the loop over
ROLES in globalSetup and replace the console.warn call that logs `[playwright
global-setup] Stored ${spec.role} session at ${spec.filePath}` with console.info
(or console.log), keeping the skip message and its console.warn unchanged; no
other behavior should change.
- Around line 64-80: The regex used to extract the Keycloak form action
(actionMatch from loginHtml using
/<form[^>]*id="kc-form-login"[^>]*action="([^"]+)"/i) is order-dependent and can
fail if action appears before id; update the extraction to be order-agnostic by
either parsing loginHtml with an HTML/DOM parser (e.g.,
querySelector('form#kc-form-login').getAttribute('action')) or replace the regex
with one that matches both attributes in any order (ensure it still decodes
using decodeHtmlEntities into formAction); update the logic around
actionMatch/loginHtml/formAction accordingly so the code reliably finds the form
action regardless of attribute ordering.
---
Outside diff comments:
In `@lib/requirements/auth.ts`:
- Around line 31-37: The JSDoc for hsaId incorrectly mentions "header-trust"
actors which no longer exist after removing the 'headers' variant from
ActorSource; update the comment on the hsaId field in lib/requirements/auth.ts
to remove the "header-trust" reference so it matches the new identity model
(keep notes about null for anonymous or MCP actors and the synthetic
`mcp-client:<client_id>` value for MCP service-account tokens).
---
Nitpick comments:
In @.github/workflows/quality-checks.yml:
- Around line 106-107: The workflow's dotenv-linter action (entry named
'dotenv-linter' running "npm run dotenv:check") and the package.json script
'dotenv:check' use different exclusion patterns so CI and local runs can
diverge; update either the workflow action or the 'dotenv:check' npm script so
they use the exact same dotenv-linter exclude globs (e.g., root exclusions like
.env.sqlserver, .env.local, .env.*.local and devcontainer exclusions like
.devcontainer/.env and .devcontainer/elevated/.env) — locate the 'dotenv:check'
script in package.json and the 'dotenv-linter' entry in the workflow (name:
'dotenv-linter') and make their exclusion flags identical to ensure parity
between local and CI linting.
In `@dev/keycloak/realm-kravhantering-dev.json`:
- Around line 129-179: The docs lack mention of the new Keycloak client
"kravhantering-local" (clientId: kravhantering-local, secret:
local-kc-app-secret, port: 3001) added for the prodlike/local-prod flow; update
docs/auth-developer-workflow.md (and optionally docs/auth-how-it-works.md) to
document this client: where to configure redirect URIs
(http://localhost:3001/api/auth/callback and
http://127.0.0.1:3001/api/auth/callback), webOrigins, the client secret, and
that this client is used for the prodlike target when running locally so
developers know which clientId/secret and port to point their local app at.
- Around line 129-179: The prod-bundle verifier doesn't currently block the new
local client id/secret from the Keycloak JSON; update the FORBIDDEN list in
scripts/verify-prod-bundle.mjs to include "kravhantering-local" and
"local-kc-app-secret" so the verifier will catch any accidental static embedding
of those dev-only values; ensure the additions are added to the same FORBIDDEN
constant/array that already contains "kravhantering-app" and "dev-only" and run
the verifier tests.
In `@docs/auth-how-it-works.md`:
- Around line 28-31: Reflow the sentence in docs/auth-how-it-works.md to remove
the awkward mid-sentence colon and make it run smoothly; replace the
two-sentence/colon form that starts with "proxy.ts is the front door. Auth is
always on, so it:" with a single sentence like "Auth is always on, so the proxy
(`proxy.ts`) allows public paths, redirects unauthenticated browser page
requests to `/api/auth/login`, returns `401` for unauthenticated API requests,
and requires a Bearer header for `/api/mcp`." Ensure the reference to proxy.ts
and the endpoint paths remain accurate.
In `@lib/requirements/auth.ts`:
- Around line 175-208: The call to getAuthConfig() in getActorContextFromSession
is currently only for its side effect (ensuring auth env vars) and looks like
dead code; make the intent explicit by replacing the bare call with either (a)
capture and use its result (e.g., const authConfig = getAuthConfig(); /* use
authConfig or pass to helper */) or (b) extract a clearly named helper like
assertAuthConfigLoaded() and call that from getActorContextFromSession so
readers know the call is intentional and won’t remove it; update the code paths
that rely on auth config accordingly (references: getActorContextFromSession,
getAuthConfig, consider adding assertAuthConfigLoaded).
In `@lib/runtime/build-target.prod.ts`:
- Around line 16-26: The constants ALLOW_INSECURE_OIDC_ISSUER,
USE_INSECURE_COOKIE, and USE_DEV_CSP are annotated as boolean which widens them
away from the literal false; change their types to the literal false (e.g.,
export const USE_DEV_CSP: false = false) or use const assertions so TypeScript
narrows branches at compile time (leave BUILD_TARGET as 'prod'); apply the same
literal-typing pattern symmetrically across the other build-target files so the
module shape remains compatible.
In `@package.json`:
- Line 20: The start:prodlike npm script redundantly prepends dotenv -e
.env.prodlike -- to npm run build:local-prod even though build:local-prod
already applies dotenv; remove the outer dotenv wrapper so start:prodlike simply
runs npm run build:local-prod && dotenv -e .env.prodlike -- cross-env
NODE_ENV=production BUILD_TARGET=local-prod next start --hostname 0.0.0.0 --port
3001, leaving the second dotenv before next start intact; update the
"start:prodlike" entry in package.json and verify the script name
"build:local-prod" remains unchanged.
In `@playwright.config.ts`:
- Around line 62-73: Normalize the Origin header by deriving it from the env var
using URL parsing instead of reusing PLAYWRIGHT_BASE_URL verbatim: compute an
origin value via new URL(process.env.PLAYWRIGHT_BASE_URL).origin (with a
try/catch and fallback to 'http://localhost:3000') and use that for
extraHTTPHeaders. Update the extraHTTPHeaders Origin assignment (and keep
baseURL as is) so Origin is the canonical origin string; reference
extraHTTPHeaders, PLAYWRIGHT_BASE_URL, and baseURL and ensure this matches the
same-origin checks in lib/auth/csrf.ts.
In `@playwright.prodlike.config.ts`:
- Around line 29-53: Add a guard in the global setup to check for the pinned
storage state file and surface a clear error when it's missing: in
tests/integration/global-setup.ts (the globalSetup function), detect when
process.env.PLAYWRIGHT_SKIP_WEBSERVER is truthy and verify the file
'test-results/auth/admin.json' exists before proceeding; if the file is not
found, throw or log an explicit error that tells the contributor to run the dev
IdP (e.g., "run `npm run idp:up` and re-run tests") so specs don't fail with an
opaque ENOENT.
In `@scripts/verify-prod-bundle.mjs`:
- Around line 41-115: The verification logic in verifyBuildTargetProd is pure
and should be extracted into a testable helper; create and export a function
(e.g. compareProdExports(actualExports, expected = EXPECTED_PROD_EXPORTS)) that
returns an object like { extras, missing, mismatched } where mismatched lists
keys with differing values, then refactor verifyBuildTargetProd to call this
helper and use its results to set failed/log messages; keep the existing
EXPECTED_PROD_EXPORTS constant and ensure the new function is exported so Vitest
unit tests can assert the three failure modes without filesystem or Node import
side effects.
- Around line 152-175: The grep invocation is brittle and uses shell quoting via
JSON.stringify and regex mode; replace execSync with child_process.spawnSync to
call grep directly and pass args as an array including '-F' and '-r' and
'--include=*.js' plus the token as a separate argument, iterating over FORBIDDEN
and [SERVER_DIR, STATIC_DIR] as before; treat exit status 1 as "not found" (no
output) and on exit code 0 read stdout (use encoding: 'utf8') to populate
output, preserving the same logging and setting of failed when output is
non-empty so token hits are reported.
In `@tests/unit/auth-me-route.test.ts`:
- Around line 7-34: Remove the unused getAuthConfig mock and its uses: delete
the vi.mock('@/lib/auth/config', ...) block and remove any
getAuthConfigMock.mockReturnValue({}) calls in the tests for the GET handler;
keep only the mocks for getSession and isSignedIn (getSessionMock,
isSignedInMock) which are actually used by the GET function in
app/api/auth/me/route. Ensure tests still call GET() and assert headers/body
unchanged after removing getAuthConfig-related setup.
In `@tests/unit/build-target.test.ts`:
- Around line 67-74: The test "contains no process.env reads" currently only
detects dotted access and misses bracket notation and aliases; update the
assertion in tests/unit/build-target.test.ts to use a broader pattern that
matches process.env followed by either a dot property access, a bracketed string
property access, or any bare "process.env" occurrence (which would indicate
aliasing/destructuring), and assert that the source of
lib/runtime/build-target.prod.ts does not match that broader pattern so bracket
notation and aliasing regressions are caught.
In `@tests/unit/mcp-token.test.ts`:
- Around line 83-197: The tests still include the obsolete enabled property in
getAuthConfigMock.mockReturnValue calls, causing inconsistent mocks; remove
enabled: true from every getAuthConfigMock.mockReturnValue usage in this test
file (used in the blocks that set issuerUrl/apiAudience in the
verifyMcpBearerToken tests and the security audit events beforeEach) so the
mocked AuthConfig matches the new contract—look for
getAuthConfigMock.mockReturnValue({...}) in the tests around the
verifyMcpBearerToken specs and the beforeEach and delete the enabled: true
entries.
In `@vitest.config.ts`:
- Around line 71-90: The alias ordering causes the general '@' alias to shadow
the more specific '@/lib/runtime/build-target' rule; move the
'@/lib/runtime/build-target' entry so it appears before the '@' alias in the
resolve.alias object in vitest.config.ts so the specific alias is matched first
(ensure the keys '@/lib/runtime/build-target' and '@' are reordered
accordingly).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3763b4d-15bb-4ee0-ba8e-19bae9743167
📒 Files selected for processing (51)
.env.development.env.example.env.prodlike.github/workflows/quality-checks.ymlapp/api/auth/callback/route.tsapp/api/auth/login/route.tsapp/api/auth/logout/route.tsapp/api/auth/me/route.tscomponents/AuthMenu.tsxcspell.jsoncdev/keycloak/realm-kravhantering-dev.jsondocs/arkitekturbeskrivning-kravhantering.mddocs/auth-developer-workflow.mddocs/auth-how-it-works.mddocs/mcp-server-contributor-guide.mdlib/auth/config.tslib/auth/csrf.tslib/auth/login-state.tslib/auth/mcp-token.tslib/auth/oidc.tslib/auth/session.tslib/mcp/http.tslib/requirements/auth.tslib/runtime/build-target.local-prod.tslib/runtime/build-target.prod.tslib/runtime/build-target.tsnext.config.tspackage.jsonplaywright.config.tsplaywright.guide.config.tsplaywright.prodlike.config.tsproxy.tsscripts/verify-prod-bundle.mjstests/integration/auth-login.spec.tstests/integration/global-setup.tstests/quality/functional.test.tstests/unit/auth-callback-audit.test.tstests/unit/auth-config.test.tstests/unit/auth-logout-audit.test.tstests/unit/auth-me-route.test.tstests/unit/auth-session-diagnostics.test.tstests/unit/auth-session.test.tstests/unit/build-target.test.tstests/unit/mcp-http.test.tstests/unit/mcp-token.test.tstests/unit/proxy.test.tstests/unit/requirements-auth-session-errors.test.tstests/unit/requirements-auth.test.tstests/unit/requirements-service.test.tsvitest.config.tsvitest.setup.ts
💤 Files with no reviewable changes (8)
- app/api/auth/login/route.ts
- app/api/auth/callback/route.ts
- tests/unit/auth-callback-audit.test.ts
- .env.development
- tests/unit/auth-logout-audit.test.ts
- app/api/auth/me/route.ts
- tests/unit/auth-session.test.ts
- tests/unit/auth-session-diagnostics.test.ts
…verification script Co-authored-by: Copilot <copilot@github.com>
…update comments for clarity
…n handling in unit tests
…L in Playwright config
…iguration for local and deployed environments
…role storage state validation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/auth-how-it-works.md (1)
298-306: Consider adding explicit rebuild semantics for build-target auth constants.This section is correct, but add one sentence that changing build target/auth behavior requires a rebuild (not runtime env toggling) to avoid operational confusion.
Based on learnings: Do not leave auth behaviour changes documented only in work documents; ensure they are reflected in
docs/auth-how-it-works.mdordocs/auth-developer-workflow.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/auth-how-it-works.md` around lines 298 - 306, Add a single clarifying sentence after the paragraph about the "insecure-issuer allowance" and "build-target constant" that states these auth-related build-target constants are baked into the build and changing them requires rebuilding (not toggling runtime env vars); reference the "insecure-issuer" and "build-target constant" terms and, for context, mention the "local-prod" / "npm run start:prodlike" scenario so readers understand this is a build-time change rather than a runtime toggle.tests/integration/global-setup.ts (1)
175-180: Catch-all error wrapping can mislead on auth-level failures.Every failure from
loginAndSaveStorageState(e.g., wrong password producing a non-2xx Keycloak response, or/api/auth/mereportingauthenticated=false) is rewrapped with "Make sure the IdP is running … and the dev server is reachable at ${baseUrl}". The underlying message is appended, but the headline points contributors at infrastructure when the real cause may be credentials or session wiring. Consider only adding the reachability hint for connection-class failures (e.g.,ECONNREFUSED, fetch errors), or splitting the message so the inner reason is the headline.next.config.ts (1)
38-43: Optional: tighten thebuildTargetcast and skip unused path computation indev.Two small polish items, both non-blocking:
buildTarget as 'dev' | 'local-prod' | 'prod'(line 38) is safe given the preceding throws, but TS doesn't narrow throughArray.prototype.includeson astring[]. Aas consttuple +Array.includestyped asreadonly BuildTarget[](or a small type guard) would let TS narrow without a cast and would keep the suffix logic in sync if a new target is added.buildTargetModulePathis computed on every config load, but only consumed whenresolvedBuildTarget !== 'dev'. Hoisting thefileURLToPath/new URLwork into theifbranch inwebpack()(or a lazy getter) avoids the wasted resolution fornext dev.♻️ Sketch
-const resolvedBuildTarget = buildTarget as 'dev' | 'local-prod' | 'prod' -const buildTargetSuffix = - resolvedBuildTarget === 'dev' ? '' : `.${resolvedBuildTarget}` -const buildTargetModulePath = fileURLToPath( - new URL(`./lib/runtime/build-target${buildTargetSuffix}.ts`, import.meta.url), -) +const BUILD_TARGETS = ['dev', 'local-prod', 'prod'] as const +type BuildTarget = (typeof BUILD_TARGETS)[number] +const isBuildTarget = (v: unknown): v is BuildTarget => + typeof v === 'string' && (BUILD_TARGETS as readonly string[]).includes(v) +// (replace the validation block with a single `!isBuildTarget(buildTarget)` check) +const resolvedBuildTarget: BuildTarget = isBuildTarget(buildTarget) + ? buildTarget + : 'dev'…and move the
fileURLToPath(...)call inside thewebpack()if (resolvedBuildTarget !== 'dev')branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next.config.ts` around lines 38 - 43, Tighten the build target typing and avoid computing the module path in dev: replace the broad `buildTarget as 'dev' | 'local-prod' | 'prod'` cast by using a const tuple (e.g. readonly BuildTargets) with Array.includes typed to narrow or add a small type guard so `resolvedBuildTarget` is properly inferred without an unsafe cast; then move the `fileURLToPath(new URL(...))` work that produces `buildTargetModulePath` out of the top-level and into the `webpack()` branch (or a lazy getter) so the `fileURLToPath`/`new URL` call only runs when `resolvedBuildTarget !== 'dev'` and `buildTargetSuffix` logic remains consistent with the typed targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integration-tests.yml:
- Around line 65-80: Update project docs to explicitly state that the
integration-tests GitHub Actions workflow requires a running local Keycloak for
both matrix legs (dev and prodlike) so CI consumers know the hard dependency; in
the workflow steps named "Start local IdP service" and "Wait for IdP to be
ready" replace the inline curl retry loop (curl -sf
http://localhost:8080/realms/kravhantering-dev/.well-known/openid-configuration)
with either a Docker-level healthcheck and use docker compose up -d --wait or at
minimum add --max-time 5 to each curl attempt to bound hangs, and mention the
idp:up / idp:down commands and the new CI requirement in the documentation text
so local and CI setup are consistent.
---
Nitpick comments:
In `@docs/auth-how-it-works.md`:
- Around line 298-306: Add a single clarifying sentence after the paragraph
about the "insecure-issuer allowance" and "build-target constant" that states
these auth-related build-target constants are baked into the build and changing
them requires rebuilding (not toggling runtime env vars); reference the
"insecure-issuer" and "build-target constant" terms and, for context, mention
the "local-prod" / "npm run start:prodlike" scenario so readers understand this
is a build-time change rather than a runtime toggle.
In `@next.config.ts`:
- Around line 38-43: Tighten the build target typing and avoid computing the
module path in dev: replace the broad `buildTarget as 'dev' | 'local-prod' |
'prod'` cast by using a const tuple (e.g. readonly BuildTargets) with
Array.includes typed to narrow or add a small type guard so
`resolvedBuildTarget` is properly inferred without an unsafe cast; then move the
`fileURLToPath(new URL(...))` work that produces `buildTargetModulePath` out of
the top-level and into the `webpack()` branch (or a lazy getter) so the
`fileURLToPath`/`new URL` call only runs when `resolvedBuildTarget !== 'dev'`
and `buildTargetSuffix` logic remains consistent with the typed targets.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dbd503b-60d7-47b6-9d61-9efbe0ead9c4
📒 Files selected for processing (19)
.github/workflows/integration-tests.yml.github/workflows/quality-checks.ymlapp/api/auth/logout/route.tscspell.jsoncdocs/arkitekturbeskrivning-kravhantering.mddocs/auth-developer-workflow.mddocs/auth-how-it-works.mdlib/requirements/auth.tslib/runtime/build-target.prod.tsnext.config.tspackage.jsonplaywright.config.tsplaywright.prodlike.config.tstests/integration/global-setup.tstests/unit/auth-logout-audit.test.tstests/unit/auth-me-route.test.tstests/unit/build-target.test.tstests/unit/mcp-token.test.tstests/unit/playwright-global-setup.test.ts
💤 Files with no reviewable changes (1)
- tests/unit/auth-me-route.test.ts
✅ Files skipped from review due to trivial changes (2)
- cspell.jsonc
- tests/unit/auth-logout-audit.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- app/api/auth/logout/route.ts
- lib/runtime/build-target.prod.ts
- .github/workflows/quality-checks.yml
- tests/unit/build-target.test.ts
- tests/unit/mcp-token.test.ts
- package.json
- playwright.prodlike.config.ts
- docs/arkitekturbeskrivning-kravhantering.md
- playwright.config.ts
- docs/auth-developer-workflow.md
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
17 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.
Description
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is