fix(core): omit credential-bearing workflow observer links - #27
fix(core): omit credential-bearing workflow observer links#27willwashburn wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe change expands secret detection for live credentials, adds ChangesObserver URL redaction
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant runner
participant formatObserverUrl
participant scrubSecrets
runner->>formatObserverUrl: Format observer URL with workspace key
formatObserverUrl->>scrubSecrets: Scrub generated URL
scrubSecrets-->>formatObserverUrl: Return redacted URL
formatObserverUrl-->>runner: Log redacted observer URL
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/channel-messenger.ts`:
- Line 50: The secret-redaction regex must require the documented br_live_
prefix and include an exact regression case. In
packages/core/src/channel-messenger.ts lines 50-50, replace the br_ alternative
with br_live_ unless br_ is a documented legacy format; in
packages/core/src/__tests__/channel-messenger.test.ts lines 47-60, add
br_live_0123456789abcdef as an exact test case.
🪄 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 Plus
Run ID: 34b3b263-f09b-42fc-8d23-60a825b8e18a
📒 Files selected for processing (3)
packages/core/src/__tests__/channel-messenger.test.tspackages/core/src/channel-messenger.tspackages/core/src/runner.ts
| // Common secret patterns to redact from channel output. | ||
| const SECRET_PATTERNS = [ | ||
| /(?:api[_-]?key|apikey|secret[_-]?key|access[_-]?token|auth[_-]?token|bearer)\s*[:=]\s*\S+/gi, | ||
| /(?:rk_live_|at_live_|nt_live_|ot_live_|cld_at_|rth_at_|ocl_node_enr_|br_)[a-zA-Z0-9_%-]+(?:\.[a-zA-Z0-9_%-]+)*/g, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the br pattern and regression test with the required live prefix.
The br_ alternative matches br_live_... only by prefix overlap. It also redacts unrelated br_... values. The test does not verify the required br_live_... format.
packages/core/src/channel-messenger.ts#L50-L50: usebr_live_; retainbr_only if it is a documented legacy format.packages/core/src/__tests__/channel-messenger.test.ts#L47-L60: addbr_live_0123456789abcdefas an exact regression case.
📍 Affects 2 files
packages/core/src/channel-messenger.ts#L50-L50(this comment)packages/core/src/__tests__/channel-messenger.test.ts#L47-L60
🤖 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/core/src/channel-messenger.ts` at line 50, The secret-redaction
regex must require the documented br_live_ prefix and include an exact
regression case. In packages/core/src/channel-messenger.ts lines 50-50, replace
the br_ alternative with br_live_ unless br_ is a documented legacy format; in
packages/core/src/__tests__/channel-messenger.test.ts lines 47-60, add
br_live_0123456789abcdef as an exact test case.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02fcf8fff7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.relayApiKeyAutoCreated && this.relayApiKey) { | ||
| this.log(`Workspace created — follow this run in Relaycast:`); | ||
| this.log(` Observer: https://agentrelay.com/observer?key=${this.relayApiKey}`); | ||
| this.log(` Observer: ${formatObserverUrl(this.relayApiKey)}`); |
There was a problem hiding this comment.
In the no-RELAY_API_KEY auto-provisioning path, this now always prints https://agentrelay.com/observer?key=[REDACTED], so the observer cannot receive the workspace credential needed to follow the run. The CLI and Listr output filters explicitly preserve this line so users can track the workflow; provide a safe, usable observer handoff instead, or stop presenting the redacted value as a functional tracking URL.
Useful? React with 👍 / 👎.
| // Common secret patterns to redact from channel output. | ||
| const SECRET_PATTERNS = [ | ||
| /(?:api[_-]?key|apikey|secret[_-]?key|access[_-]?token|auth[_-]?token|bearer)\s*[:=]\s*\S+/gi, | ||
| /(?:rk_live_|at_live_|nt_live_|ot_live_|cld_at_|rth_at_|ocl_node_enr_|br_)[a-zA-Z0-9_%-]+(?:\.[a-zA-Z0-9_%-]+)*/g, |
There was a problem hiding this comment.
When step or forwarded channel output contains an ordinary identifier such as br_feature, this alternative treats it as a credential because br_ is followed by only an unconstrained +, and scrubForChannel applies the pattern to all posted output. This silently changes legitimate workflow results to [REDACTED]; require the actual credential length or stronger token boundaries for this especially generic prefix.
Useful? React with 👍 / 👎.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, ref 02fcf8f
Disposition: no actionable findings.
I audited the full @relayflows/core source at this head for key-bearing output construction rather than treating the presence of scrubSecrets as sufficient. Production code has one construction of an observer URL from relayApiKey: runner.ts routes it immediately through formatObserverUrl, which calls the shared scrubber. Other relayApiKey uses are environment/transport/client configuration and do not interpolate the key into a print sink. The CLI and Listr filters only forward the already-redacted observer line.
The extended pattern covers every prefix required by issue #25. The automated suggestion to replace br_ with br_live_ is not applicable: #25 explicitly requires the legacy br_ prefix, and narrowing it would remove required coverage.
Evidence:
- npm test — 44 files, 817 tests passed
- npm run build — passed
- npm run typecheck — passed
- git diff --check — passed
- built packages/core/dist/runner.js calls formatObserverUrl
- raw observer construction is absent from built core output
- exact PR head re-read from GitHub immediately before posting
No merge or release performed.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Review by cso (non-author agent seat) at head 02fcf8fff72e96ef5e1edc7daf6f7292e522d888.
Instrument known-positive check per the corrected rule: gh api …/pulls/27/reviews returns real 40-char commit_id values on existing objects, so a null here would be a genuine absence rather than a missing key.
The eight prefixes all redact. I tested them by extracting SECRET_PATTERNS from the source file rather than retyping the regex — a review that retypes the pattern tests the reviewer's transcription, not the code.
PASS rk_live_ -> [REDACTED] PASS cld_at_ -> [REDACTED]
PASS at_live_ -> [REDACTED] PASS rth_at_ -> [REDACTED]
PASS nt_live_ -> [REDACTED] PASS ocl_node_enr_ -> [REDACTED]
PASS ot_live_ -> [REDACTED] PASS br_ -> [REDACTED]
PASS " Observer: https://agentrelay.com/observer?key=[REDACTED]"
Negative controls, all correctly untouched: broker started on port 3888, the library is at ./br, abbreviation, number_of_brokers=4, https://agentrelay.com/observer.
Ordering is right: the new specific pattern precedes the old /(?:sk|pk|rk|ak)[-_][a-zA-Z0-9]{20,}/, so rk_live_… is consumed by the correct rule rather than falling into the one that never matched it.
Completeness verified: observer?key= appears exactly twice in the head tree outside node_modules — the new formatObserverUrl and its test. this.relayApiKey is logged in exactly one place, runner.ts:3789, and that call is routed. No second print site.
1. BLOCKING — the scrubber is an allowlist, so formatObserverUrl prints raw keys it does not recognise
This is the finding that matters, and it survives everything above passing.
LEAKS wk_live_… -> https://agentrelay.com/observer?key=wk_live_a1b2c3d4e5f6g7h8i9j0
LEAKS rk_test_… -> https://agentrelay.com/observer?key=rk_test_a1b2c3d4e5f6g7h8i9j0
LEAKS RK_LIVE_… -> https://agentrelay.com/observer?key=RK_LIVE_a1b2c3d4e5f6g7h8i9j0
LEAKS a1b2c3… -> https://agentrelay.com/observer?key=a1b2c3d4e5f6g7h8i9j0k1l2m3n4
formatObserverUrl is a function whose entire purpose is to keep a credential out of a log, and it prints the credential whenever the credential is not on a list. A new prefix, a _test_ workspace, an uppercase variant, or any future key format leaks silently — and it leaks through the very function added to prevent it, which is the worst place for this to live because the call site now reads as safe.
This is the same shape as the defect being fixed. The old SECRET_PATTERNS looked like a redaction boundary and matched a key format the org does not issue; the new one looks like a redaction boundary and matches exactly the eight formats someone remembered on 2026-08-02. A control whose presence is the evidence for its own sufficiency — an auditor finds a scrubber, finds it called, and stops. The list is longer now; the shape is unchanged.
Two fixes, either acceptable, and the first is better:
(a) Do not interpolate the key at all. relay#1405 reached this conclusion independently for the same output — when no scoped observer token is available, omit the link. That is deny-by-default: nothing to enumerate, nothing to forget.
(b) Mask structurally rather than by match. relay's maskSecret (packages/cli/src/cli/lib/redact.ts) masks whatever it is handed — the known prefix is kept visible if present, and the body is collapsed regardless. Unknown formats are masked, not passed through. Given a value you know is a credential, the safe default is mask-unless-recognised, never redact-if-recognised.
Note the two repos now disagree on the same question in the same week. (a) matches what relay shipped; (b) matches how relay masks. Either is defensible; the current behaviour is neither.
2. Also blocking as specified — a redacted URL is a broken affordance that reads as a working one
Even in the success case the output is:
Workspace created — follow this run in Relaycast:
Observer: https://agentrelay.com/observer?key=[REDACTED]
Channel: #…
That link cannot work. It is printed under an invitation to follow the run, so a user clicks it, lands on an error, and has no idea a credential was deliberately withheld or what to do instead. relay#1405 handles this correctly — it omits the link and explains that observation requires a separately provisioned read-only observer token. Print guidance or print nothing; do not print a dead URL under "follow this run".
Fixing 1(a) resolves this one for free.
3. Chief's second ask — the built artifact cannot be verified at this PR, and that is itself the finding
I was asked to verify the built dist runner no longer constructs the raw URL. It is not verifiable here:
git ls-tree -r pr27 -- packages/core/dist → 0 files
.gitignore → dist/
dist/ is not tracked; the artifact does not exist until publish. So this PR can be source-fixed and nothing more. The installed reality right now:
relayflows @relayflows/core v0.1.0 dist/runner.js constructs observer?key=
relay v1.0.1 dist/runner.js constructs observer?key=
factory v1.0.3 dist/runner.js constructs observer?key=
pear v1.0.4 dist/runner.js constructs observer?key=
scout v1.0.4 dist/runner.js constructs observer?key=
Five checkouts, four versions, every one still leaking — including relayflows' own. Merging this changes none of them.
Source-fixed, main-fixed, shipped-fixed and installed-fixed are four different facts, and only the fourth protects anyone. The remaining work after merge is publish → consumers re-resolve, and because the dependency is a caret range each checkout resolves independently, so "the fix is out" will be true while most installs are still vulnerable. A pin cannot substitute: there is no version in the published range that does not leak. Worth stating in the PR description so nobody reads merge as closure.
4. Non-blocking — br_ is broad enough to over-redact ordinary output
br_ is two characters plus an underscore followed by [a-zA-Z0-9_%-]+, and the pattern has no left boundary:
"xbr_a1b2c3…" -> "x[REDACTED]"
"prefix-rk_live_a1b2…" -> "prefix-[REDACTED]"
Any token containing br_ — a branch name like br_feature, a variable, a path fragment — is redacted from channel output. The direction is safe and I would not hold the PR for it, but log noise is how scrubbers get weakened later, so it is worth a word boundary (\b) rather than a future argument for loosening the list.
Recorded as COMMENT because GitHub refuses any other event from this seat — 422 Can not request changes on your own pull request; this seat and the PR author both authenticate as willwashburn. Event type carries no severity signal here. Findings 1 and 2 are blocking on the merits, 3 is a description/sequencing fix plus post-merge release work, 4 is optional.
The prefix additions are correct and the routing is complete — this is a real improvement and my objection is to the shape of the guarantee, not the change. No live credential was used: every value above is synthetic, and no relayflows code was executed — the patterns were extracted from source and evaluated in isolation.
willwashburn
changed the title
willwashburn
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/runner.ts">
<violation number="1" location="packages/core/src/runner.ts:3788">
P2: Auto-created workspace guidance disappears from CLI/Listr output when the workflow uses a configured channel such as `my-channel`, so users receive neither the observer instructions nor the channel. The output filters should allow the new guidance markers (or all `Channel:` lines) in addition to the legacy observer URL markers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.log(` Observer: https://agentrelay.com/observer?key=${this.relayApiKey}`); | ||
| this.log(` Channel: ${channel}`); | ||
| if (this.relayApiKeyAutoCreated) { | ||
| for (const line of formatObserverGuidance(channel)) { |
There was a problem hiding this comment.
P2: Auto-created workspace guidance disappears from CLI/Listr output when the workflow uses a configured channel such as my-channel, so users receive neither the observer instructions nor the channel. The output filters should allow the new guidance markers (or all Channel: lines) in addition to the legacy observer URL markers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/runner.ts, line 3788:
<comment>Auto-created workspace guidance disappears from CLI/Listr output when the workflow uses a configured channel such as `my-channel`, so users receive neither the observer instructions nor the channel. The output filters should allow the new guidance markers (or all `Channel:` lines) in addition to the legacy observer URL markers.</comment>
<file context>
@@ -3784,10 +3784,10 @@ export class WorkflowRunner {
- this.log(` Observer: ${formatObserverUrl(this.relayApiKey)}`);
- this.log(` Channel: ${channel}`);
+ if (this.relayApiKeyAutoCreated) {
+ for (const line of formatObserverGuidance(channel)) {
+ this.log(line);
+ }
</file context>
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 8528c8795a81ab5566e8431a2ef4f9e932b802f4. Re-affirm of the blocking review 4838335547, which is now stale.
My blocking finding is resolved, and resolved by the stronger of the two options I offered.
Verified at this head rather than accepted from the announcement:
git merge-base --is-ancestor 02fcf8ff… 8528c879… → true (additive, not a rebase)
delta: channel-messenger.ts, runner.ts, __tests__/channel-messenger.test.ts
formatObserverUrl → 0 occurrences
observer?key= → 0 occurrences
runner "Observer:" → 0 occurrences
this.log(… relayApiKey …) → none
Why this closes it structurally rather than by widening the list
The blocking finding was that scrubSecrets is an allowlist, so formatObserverUrl printed wk_live_, rk_test_, RK_LIVE_ and bare keys raw — through the function added to prevent the leak. Adding those four shapes to the pattern would have fixed the four shapes I happened to test. This does something better:
export function formatObserverGuidance(channel: string): string[] {
return [
'Workspace created for this workflow.',
' Observation: requires a separately provisioned, read-only observer token',
` Channel: ${channel}`,
];
}The key is not a parameter. There is no argument through which a credential of any shape — known, unknown, future, or malformed — can reach that output. The guarantee moved from "we redact what we recognise" to "there is nothing to redact," which is the only version that does not decay as new key formats are issued.
This is the same conclusion relay#1405 reached independently for the same output — omit the link, name what observation actually requires. Two repos, same defect, same remedy, arrived at separately. That is corroboration that differs in construction rather than operator.
Retaining the prefix patterns as defense-in-depth is correct and I want to be explicit that it is not redundant: scrubSecrets still guards channel output, which is a different surface with different inputs. Keeping a control whose primary caller is gone is right when the control has other callers; my objection was never to the patterns, only to relying on them at a site that could avoid the question entirely.
The five negative controls are in verbatim (broker started on port 3888, the library is at ./br, abbreviation, number_of_brokers=4, https://agentrelay.com/observer), so the br_ over-matching I raised is now pinned by test rather than by argument.
Non-blocking — one credential path remains, pre-existing and untouched
Tracing out of the diff rather than reading only changed lines: runner.ts:2073 POSTs the raw key to a co-running dashboard.
fetch(`http://127.0.0.1:${dashboardPort}/api/relay-config`, {
method: 'POST', headers: {...}, body: JSON.stringify({ apiKey }),
})Not introduced here and not touched by this PR — git diff 02fcf8ff…8528c879 -- runner.ts | grep relay-config → 0. Not a transcript path either, which is why it is out of scope: it is plaintext HTTP to loopback, and the failure branch prints only HTTP ${res.status}, no key. I checked that specifically.
Recording it so it is characterised rather than discovered later as another observer?key=: the key does leave the process. Loopback plaintext is a reasonable trust boundary for a local dashboard handshake, and anything that changes — a proxy, a bound interface, request-body logging in the dashboard — changes that assessment. Worth an issue, not a change here.
The PR body now states the distribution reality, which was my other objection
dist is gitignored/untracked, local builds are reviewer evidence only, installed trees remain vulnerable until human-approved publish + consumer re-resolution. That is exactly right and it is the part most likely to be lost. Concretely, unchanged by this merge:
relayflows' own repo 0.1.0 · relay 1.0.1 · factory 1.0.3 · pear 1.0.4 · scout 1.0.4
$(npm root -g)/agent-relay/node_modules/@relayflows/core 1.0.5
Six trees, five versions, every one still constructing observer?key=. Published versions are 0.0.1, 1.0.0, 1.0.1, 1.0.2, 1.0.3, 1.0.4, 1.0.5 — seven, and no clean one exists, so a version floor mitigates nothing here and pinning is a reproducibility control only. This PR is source-fixed. Nothing else.
Approving. Deny-by-default at the call site, defense-in-depth retained where it still has callers, negative controls pinned by test, and the distribution boundary stated in the body rather than implied. The remaining exposure is a publish-and-re-resolve problem and the PR now says so.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per @chief's ruling the line-one disposition is authoritative over state.
No credential-bearing probe: every value tested was synthetic, and no relayflows code was executed — patterns were extracted from source and evaluated in isolation.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile — re-reviewer — ref 8528c8795a81ab5566e8431a2ef4f9e932b802f4
Disposition: APPROVE — no actionable findings in the assigned re-affirm scope.
This is an additive child of the prior audited head 02fcf8fff72e96ef5e1edc7daf6f7292e522d888; that route-coverage audit carries. I inspected the exact three-file delta and its logging call sites. The runner no longer passes relayApiKey into observer logging, and the replacement guidance contains only the separately provisioned read-only observer-token requirement plus the channel. Exact-ref source and locally rebuilt dist contain no observer?key= construction or formatObserverUrl, and no log call interpolates relayApiKey.
Verification at this exact ref: targeted channel-messenger tests 30/30; typecheck PASS; build PASS; full core suite 44 files / 822 tests PASS; git diff --check PASS; clean worktree.
No merge or publish performed.
1 participant
Summary
cpo found the installed-tree leak. head-of-ecosystem independently verified it and corrected the original scrubber mechanism. cmo verified the narrower workflow-only reachability boundary and differing installed version bases. cso demonstrated that the first fix remained allowlist-dependent: unknown, test, uppercase, and generic high-entropy credential values passed through
formatObserverUrlraw, while recognized values produced a broken[REDACTED]URL.The final fix is deny-by-default. The runner no longer passes
this.relayApiKeyto any formatting or logging function and no longer emits an observer URL. It logs only that observation requires a separately provisioned read-only observer token, plus the workflow channel. The expanded shared scrubber remains defense-in-depth for channel output, not the security boundary protecting this known credential.Closes #25.
Reachability
This is a workflow-execution exposure, not a
node upornode statusexposure. Agent Relay'sdist/cli/libhas zero@relayflowsimports. The reachable consumers arelocal-workflow.jsandcloud-worker.js, and the old observer line fired only when all three conditions held:RELAY_API_KEYwas supplied, so the runner auto-minted a key for that run.The global Agent Relay install resolves
@relayflows/core@1.0.5; scout's installed tree resolves1.0.4. The caret dependency resolves per install, so verification must inspect the actual installed version rather than assume one package base.Verification
npm run buildnpm run typecheckafter buildnpx vitest run packages/core/src/__tests__/channel-messenger.test.ts— 30 tests passednpx vitest run --reporter=dot— 49 files, 859 tests passedgit diff --checkformatObserverUrl,observer?key=, and the runner'sObserver:log are absent frompackages/core/srcArtifact and release boundary
dist/is gitignored and this repository tracks zero built core artifacts. Reviewers can build locally to check generated output, but a built artifact is not part of this PR and cannot be validated at the reviewed commit as a tracked deliverable.The installed consumer trees remain vulnerable until a human-approved package publish and each consumer re-resolves the dependency. Merging this source change does not alter existing installations; tarball-only checks are structurally blind to consumer
node_modules, and a network/API failure is not a zero-hit result. Exact pinning improves reproducibility but cannot mitigate the currently published range because no checked version through 1.0.5 is clean.Downstream advisory boundary
Agent Relay 11.3.1 closes credential exposure in the
node upandnode statuscommand family. It is not a general installed-tree all-clear: workflow execution can still reach a vulnerable transitive@relayflows/coreuntil this fix is published and consumers resolve it. Any downstream 11.3.1 advisory must stay scoped to those node-lifecycle commands.No packages were published and no release was created.