observability(server): say when a request loses its project config - #3457
Conversation
codersociety's preview serves the platform-default CSP instead of the project's on roughly a third of renders, for the same URL and the same authenticated request. The two responses are byte-identical HTML; only the headers differ, because the project's two veryfront.config.ts versions differ only by a security.csp block. Nothing in production logs said so. resolveProjectRuntimeContext substitutes the process-wide SecurityConfigLoader for an absent project config. That is correct for the config-less control-plane endpoints it was written for, and silent everywhere else: the response is a 200 whose policy is the platform floor rather than the tenant's, which no downstream consumer can distinguish from a project that configured nothing. Four unrelated branches reach it -- an inherited caller config, a deliberate defer, a hosted 404, and a project that publishes no config -- and the returned `config: undefined` cannot tell them apart. Name the branch. resolveAdapter now returns a ConfigResolutionOutcome alongside the config, and the fallback logs a warn carrying it plus the source identity of the request. Deferred control-plane requests are excluded: a config-less security context is their intended shape. Also raise "No hosted config for this release; using defaults" from debug to warn. For a project that genuinely publishes no config this is routine, but it is indistinguishable here from a config that momentarily 404s, and both degrade the same way downstream. At debug it was invisible in production while a preview served the wrong CSP for hours. This is instrumentation, not the fix. The mechanism that makes the config resolve intermittently is still unknown -- the timing split is clean (resolve_adapter 57-61ms when the config is lost, 469-1091ms when it is not) but three candidate causes have already been falsified. This is what makes the next production sample conclusive. Verified: 119 files / 1525 steps across src/server/runtime-handler and src/security. deno check, lint and fmt clean on src/server/runtime-handler.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe adapter factory now reports how configuration was resolved, including absent hosted configuration. Proxy runtime context uses this outcome to warn when project security configuration is missing while excluding deferred control-plane requests. ChangesConfiguration resolution and runtime diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectRuntimeContext
participant resolveAdapter
participant SERVER
ProjectRuntimeContext->>resolveAdapter: resolve adapter and configOutcome
resolveAdapter-->>ProjectRuntimeContext: return configuration outcome
ProjectRuntimeContext->>SERVER: warn for missing non-deferred project configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/server/runtime-handler/project-runtime-context.test.ts`:
- Around line 1362-1365: Extend the test around the deferred adapter outcome
assertion to capture emitted log records, then assert that none contains
"serving platform-default security headers". Keep the existing
adapter.configOutcome "deferred" assertion unchanged and verify the warning is
excluded specifically for the config-less control-plane request.
In `@src/server/runtime-handler/project-runtime-context.ts`:
- Around line 342-350: Remove or redact the raw project and request metadata
from both warning calls: the warning in
src/server/runtime-handler/project-runtime-context.ts lines 342-350 and the
corresponding warning in src/server/runtime-handler/adapter-factory.ts lines
350-357. Eliminate project, release, branch, environment, and pathname fields,
or replace them with an approved correlation identifier while preserving the
warnings themselves.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17a24ca9-3707-40af-a815-2f5f17213299
📒 Files selected for processing (4)
src/server/runtime-handler/adapter-factory.test.tssrc/server/runtime-handler/adapter-factory.tssrc/server/runtime-handler/project-runtime-context.test.tssrc/server/runtime-handler/project-runtime-context.ts
| logger.warn("No project config for this request; serving platform-default security headers", { | ||
| projectSlug: projectRes.projectSlug, | ||
| projectId: projectRes.projectId, | ||
| configOutcome: adapterRes.configOutcome, | ||
| releaseId: envRes.releaseId ?? null, | ||
| branch: reqCtx.branch ?? null, | ||
| environmentName: projectRes.environmentName ?? null, | ||
| resolvedEnvironment: envRes.resolvedEnvironment ?? null, | ||
| pathname: input.url.pathname, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove raw project and request metadata from both warnings.
Both warning paths log tenant identifiers and request-derived values. These values can contain customer data or private infrastructure details. Use redacted values or an approved correlation identifier.
src/server/runtime-handler/project-runtime-context.ts#L342-L350: remove or redact project, release, branch, environment, and pathname fields.src/server/runtime-handler/adapter-factory.ts#L350-L357: remove or redact project, release, branch, environment, and pathname fields.
📍 Affects 2 files
src/server/runtime-handler/project-runtime-context.ts#L342-L350(this comment)src/server/runtime-handler/adapter-factory.ts#L350-L357
🤖 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 `@src/server/runtime-handler/project-runtime-context.ts` around lines 342 -
350, Remove or redact the raw project and request metadata from both warning
calls: the warning in src/server/runtime-handler/project-runtime-context.ts
lines 342-350 and the corresponding warning in
src/server/runtime-handler/adapter-factory.ts lines 350-357. Eliminate project,
release, branch, environment, and pathname fields, or replace them with an
approved correlation identifier while preserving the warnings themselves.
Source: Coding guidelines
The deferred case asserted the adapter outcome but not the warning exclusion, which is the part that actually matters. Every signed control-plane request takes that path, so a regression there would warn on all of them and bury the signal the warning exists to carry. Capture log records and assert nothing reports the security fallback.
|
Addressed the first finding in On the second finding (redact project/release/branch/environment/pathname from both warnings) — I'm not implementing this, and want to give the reasoning rather than silently skip it. The identifiers are the payload, not incidental context. This warning exists to answer one question: which project just lost its config? A production preview is serving the platform-default CSP instead of the tenant's on roughly a third of renders, and six hypotheses for why have already been falsified. It matches what the surrounding code already does. The fields aren't customer data. A project slug is a public DNS label — Happy to revisit if there's a specific field you'd single out — |
1 participant
What
resolveAdapterreturns aConfigResolutionOutcomenaming which branch producedconfig, and the security fallback logs a warn carrying it."No hosted config for this release; using defaults"goes from debug to warn.This is instrumentation, not a fix. It exists to make an unsolved bug diagnosable.
The bug it instruments
codersociety.preview.veryfront.comserves the platform-default CSP instead of the project's on roughly a third of renders — same URL, same authenticated request. The two responses are byte-identical HTML; only the headers differ, because the project's twoveryfront.config.tsversions differ only by asecurity.cspblock. Nothing in production logs says so.Reproducible in one loop:
with a clean bimodal split on
runtime.resolve_adapter— 57–61 ms when the config is lost, 469–1091 ms when it is not, no overlap across 8 samples.Why it was undiagnosable
resolveProjectRuntimeContextsubstitutes the process-wideSecurityConfigLoaderfor an absent project config. That is correct for the config-less control-plane endpoints it was written for, and silent everywhere else: the response is a 200 whose policy is the platform floor rather than the tenant's, which no downstream consumer can distinguish from a project that configured nothing.Four unrelated branches reach it — an inherited caller config, a deliberate defer, a hosted 404, and a project that publishes no config — and the returned
config: undefinedcannot tell them apart. Every one of them logged at debug.What this adds
ConfigResolutionOutcome:inherited|local|deferred|hosted|hosted-absentdeferredis excluded — a config-less security context is the intended shape for signed control-plane requests, not a degradationWhy not the fix itself
Six hypotheses for the underlying race have been falsified: content staleness (both sources now carry the CSP and it still flips), pod boot cache (fleet is on a 70-minute-old ReplicaSet, post-dating the config change), API inconsistency (12/12 consistent),
studio_embed, release-pin, and CSS recompilation. Each was killed by measurement, not argument.configOutcomediscriminates between every remaining hypothesis in a single production sample. That is the cheapest path to the actual fix.Verification
119 files / 1525 steps across
src/server/runtime-handlerandsrc/security, plus full unit suite green: 3811 passed, 0 failed.deno check,lint,fmtclean.Summary by CodeRabbit