Sr 09

[rachellerathbone]

What

Brief description of changes

Why

Why this change was needed

How

Brief technical approach

Testing

How to verify the changes

[github-actions]

multicorn-ops review

Persona	Role	Primary	Status	Summary
Jordan	Security Auditor	yes	Concern	URL allowlist logic uses prefix matching which is bypassable, and config-sourced base URLs are used before full HTTPS validation in some paths.
Priya	Open Source Contributor	yes	Passed	Good test coverage for the new base-URL resolution logic, clear naming, and the refactor to parseConfigFile() is easy to follow.
Marcus	Design-Conscious Developer	no	Passed	No UI changes in this diff; error messages are plain text only and don't affect rendered UI.
Sarah	Non-Technical Decision-Maker	no	Passed	No user-facing marketing or onboarding copy changed; error strings are technical but not customer-visible.
The Team	Acquisition Due Diligence	yes	Concern	Refactor improves config layering but the URL validation primitive is subtly unsafe, test coverage for the CLI path is incomplete, and the truncated diff prevents full assessment.
Alex	Accessibility Advocate	no	Passed	No UI or HTML changes in this diff; accessibility is unaffected.
Yuki	International User	no	Concern	Some error messages are actionable but one hides the actual bad value, which makes debugging harder for non-native speakers.

Concerns

Jordan (Security Auditor)

• src/proxy/config.ts:210 - isAllowedShieldApiBaseUrl uses startsWith('http://localhost') which also permits 'http://localhost.evil.com'. Use URL parsing to check hostname exactly equals 'localhost' or '127.0.0.1' instead of prefix matching.
• src/proxy/config.ts - readBaseUrlFromConfig returns the raw string from disk with no HTTPS validation. Callers (runInit) do validate afterward, but any future caller that forgets will silently use a non-HTTPS URL to which the API key is sent.
• src/proxy/config.ts - The MULTICORN_BASE_URL environment variable is trusted and used as-is after only the isAllowedShieldApiBaseUrl prefix check. An attacker who can set env vars could redirect traffic to http://localhost.attacker.com.
• bin/multicorn-proxy.ts:234 - When --base-url is not provided, the config-file base URL is validated only after loadConfig() succeeds. If loadConfig returns a config whose baseUrl is non-HTTPS (e.g. a tampered config file), the error message says '--base-url must use HTTPS' which is misleading and may not surface clearly in automated pipelines.
• src/proxy/__tests__/proxy.edge-cases.test.ts - The 'runInit rejects non-HTTPS base URL from config.json' test only checks that stderrBuffer contains 'HTTPS' and 'base-url', but does not assert that no network call was made with the bad URL before the rejection. A regression where the URL is used briefly before validation would not be caught.

Priya (Open Source Contributor)

• src/proxy/config.ts - DEFAULT_SHIELD_API_BASE_URL is defined inside config.ts but the old hardcoded default 'https://api.multicorn.ai' still appears in comments and tests — a CONTRIBUTING newcomer searching for the default may miss the constant.

The Team (Acquisition Due Diligence)

• src/proxy/config.ts:210 - isAllowedShieldApiBaseUrl is a naive string-prefix check exported as a security boundary. This is the kind of subtle bug that survives code review and causes a CVE later; URL parsing (new URL(url).hostname) would be far safer.
• bin/multicorn-proxy.ts - The version bump to 0.6.1 in package.json implies a patch release, but the base-URL resolution behaviour change (config file now takes precedence over the old hardcoded default) is a behaviour change for existing users. Should be documented in CHANGELOG.
• src/proxy/__tests__/proxy.edge-cases.test.ts - Tests cover runInit scenarios but there are no integration-level tests for the CLI arg-parsing path in bin/multicorn-proxy.ts — specifically the new two-stage validation (pre-loadConfig and post-loadConfig). A unit test for parseArgs returning undefined baseUrl would add confidence.
• src/proxy/config.ts - parseConfigFile silently swallows readError in loadConfig (returns null) but emits a warning in readBaseUrlFromConfig. The asymmetry could confuse operators diagnosing config problems.

Yuki (International User)

• bin/multicorn-proxy.ts:237 - Error message says 'Received a non-HTTPS URL' but no longer shows the actual URL. Showing the value (even partially redacted) helps users confirm which config source produced the bad URL.
• src/proxy/config.ts - The runInit error 'Use --base-url to override' assumes the user knows what --base-url is. Adding a short example like '--base-url https://your-server.example.com' would be more actionable for non-native English speakers.

Open-Source Readiness Checklist

Code Quality

• All functions have clear, descriptive names — New functions like isAllowedShieldApiBaseUrl, parseConfigFile, readBaseUrlFromConfig, and runInit are clearly named and descriptive.
• No hardcoded secrets, API keys, internal URLs, or employee names in code or comments — Only the public production URL https://api.multicorn.ai is hardcoded as a default constant; no secrets or internal URLs detected.
• [~] No // TODO without a public issue reference — No TODOs visible in the truncated diff; cannot confirm absence throughout entire codebase.
• No commented-out code blocks — No commented-out code blocks observed in the diff.
• No debug logging (console.log, println) left in — No console.log or debug logging statements introduced.
• All any types eliminated (TypeScript) — The diff uses unknown instead of any in new code (e.g., ParseConfigFileResult, parseConfigFile return type).
• Error handling is complete — no swallowed exceptions, no empty catch blocks — parseConfigFile handles ENOENT and generic errors explicitly; readBaseUrlFromConfig handles all error kinds with user-facing messages.
• No Atlassian-internal references, no proprietary patterns or terminology — No Atlassian-internal references found in the diff.

Testing

• All new code has tests — New functions readBaseUrlFromConfig and isAllowedShieldApiBaseUrl are covered by new tests in config.test.ts and proxy.edge-cases.test.ts. runInit base URL resolution paths are tested with four new test cases.
• [~] Coverage meets or exceeds repo minimum — Coverage metrics are not available from the diff alone; CI results would be needed.
• [~] Tests pass locally and in CI — Cannot determine CI status from the diff alone.
• Edge cases and error paths are tested — Tests cover missing file, partial config (no apiKey), non-HTTPS base URL rejection, explicit override, and brand-new install scenarios.
• [~] No flaky tests — Cannot assess flakiness from the diff alone; requires repeated CI runs.

Security

• No secrets in code, comments, config files, or git history — No secrets introduced; only a public API base URL is present.
• All user input is validated — isAllowedShieldApiBaseUrl validates base URLs from CLI and config before any network calls. The validation is applied both to the explicit CLI flag and to the resolved URL from config.
• [~] Dependencies audited — no known vulnerabilities — No dependency changes in the diff; cannot audit from diff alone.
• HTTPS enforced for all external communication — isAllowedShieldApiBaseUrl enforces HTTPS (with localhost exception), and is applied before any network calls in both the CLI entry point and runInit.
• API keys/tokens never logged — No API key logging introduced; error messages omit the actual base URL value in some places (e.g., 'Received a non-HTTPS URL' rather than printing the value).

Documentation

• [~] README.md is accurate and up to date — README.md not included in the diff.
• [~] CONTRIBUTING.md is accurate and up to date — CONTRIBUTING.md not included in the diff.
• [~] CHANGELOG.md updated with this change — CHANGELOG.md not included in the diff.
• New public APIs have JSDoc/KDoc with examples — readBaseUrlFromConfig, isAllowedShieldApiBaseUrl, parseConfigFile, and the updated runInit all have JSDoc comments. Examples are not included but descriptions and param docs are present.
• [~] Any new config options are documented — No new config options introduced; baseUrl was already a config field. README not in diff to verify.
• [~] Architecture decisions documented in ADR if significant — The change is a refactor of URL resolution priority. No ADR is visible in the diff; unclear if one is required by repo policy.

Open Source Hygiene

• [~] Licence header present in source files (if required by licence) — No licence headers present in the modified files, but cannot determine if the repo requires them from the diff alone.
• [~] CODE_OF_CONDUCT.md present — Not assessable from the diff.
• [~] Issue templates are current — Not assessable from the diff.
• [~] PR template is current — Not assessable from the diff.
• No internal company references or links — Only public URLs (api.multicorn.ai) and example domains are referenced.
• Package name and description are correct in package.json — Version bumped from 0.6.0 to 0.6.1; name is multicorn-shield and description is unchanged and appropriate.
• [~] Repository topics/tags are set on GitHub — Cannot determine GitHub repository topics from the diff.

---

Advisory only. Does not block merge. Actions logged to Shield as pr_review and oss_check.