Fix review comments: AASA content-type, proxy header spoofing, API_BASE_URL const mutation, installer guard, docs#143
Conversation
… template check, API_BASE_URL fix, docs update Co-authored-by: elifouts <116454864+elifouts@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes five issues flagged in a previous code review for the AWS-ready PR. The changes address content-type headers for app-link validation files, proxy header spoofing prevention, an incorrect const mutation pattern in TypeScript, a missing file guard in the systemd installer script, and outdated documentation.
Changes:
- Content-Type fix for AASA/assetlinks: Replaced
StaticFilehandlers forapple-app-site-associationandassetlinks.jsonwith explicitGEThandlers that setContent-Type: application/json; also adds the/.well-known/apple-app-site-associationalias. - Proxy header spoofing fix:
X-Forwarded-Protois now only honored whenDEVBITS_TRUST_PROXY=true, controlled by a package-level variable evaluated at startup; the.env.exampleis updated accordingly. - TypeScript and docs fixes: Replaces the no-op
(exports as any).API_BASE_URL = ...mutation pattern with a propergetValidatedBaseUrl()function; updatesDEEP_LINK_SETUP.mdto reference the backend static directory; adds a template-existence guard to the systemd installer; and promotesgodotenvfrom indirect to direct dependency.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
backend/api/main.go |
Replaces StaticFile with explicit GET handlers that set Content-Type: application/json; adds /.well-known/apple-app-site-association alias |
backend/api/internal/handlers/media_routes.go |
Adds trustProxy package-level var to gate X-Forwarded-Proto header trust on DEVBITS_TRUST_PROXY=true |
frontend/services/api.ts |
Replaces no-op (exports as any).API_BASE_URL = ... mutation with a proper getValidatedBaseUrl() function |
frontend/DEEP_LINK_SETUP.md |
Updates static file paths from frontend/public/ to backend/api/static/; documents new /.well-known/apple-app-site-association alias |
backend/scripts/install-aws-systemd-service.sh |
Adds [[ ! -f "$TEMPLATE_PATH" ]] guard with a clear error before the sed invocation |
backend/go.mod |
Promotes godotenv from indirect to direct dependency |
backend/.env.example |
Documents the new DEVBITS_TRUST_PROXY environment variable |
Critical issue found:
The trustProxy package-level variable in media_routes.go (line 26) is initialized by calling os.Getenv("DEVBITS_TRUST_PROXY") at package init time, before godotenv.Load() is called in main() (line 87 of main.go). This means if DEVBITS_TRUST_PROXY=true is set in the .env file — which is the expected mechanism for local development and is explicitly shown in the new .env.example — the value is never picked up, and the variable is silently always false. In production (where the env var is set at the OS level via systemd EnvironmentFile), it works correctly, but the .env-based development workflow is broken.
The codebase consistently reads environment variables inside functions called at request time (e.g., getSecret() in auth/jwt.go:21 and RequireAdmin() in auth_middleware.go:43), not at package init. The trustProxy variable should either be read inside UploadMedia per-request, or initialized in main() after godotenv.Load() and passed into the handler.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // trustProxy is evaluated once at startup to avoid repeated os.Getenv calls | ||
| // on every upload request. Set DEVBITS_TRUST_PROXY=true only when the backend | ||
| // runs behind a trusted reverse proxy (e.g. AWS ALB) that sets X-Forwarded-Proto. | ||
| var trustProxy = os.Getenv("DEVBITS_TRUST_PROXY") == "true" |
There was a problem hiding this comment.
The trustProxy variable is initialized as a package-level var by calling os.Getenv("DEVBITS_TRUST_PROXY") at package init time. However, godotenv.Load(".env", ...) is called inside main(), which runs after all package-level variable initialization. This means if DEVBITS_TRUST_PROXY=true is set in the .env file (as is the case for local development), the value will never be seen by trustProxy — it will always evaluate to false, silently broken.
Other env-dependent values in the codebase (e.g. DEVBITS_JWT_SECRET in auth/jwt.go:21, DEVBITS_ADMIN_KEY in auth_middleware.go:43) call os.Getenv inside functions at request time, not at package init, which is why they work correctly. The trustProxy variable should either be read inside the handler function per-request, or be initialized in main() after godotenv.Load() and injected into the handler.
| // trustProxy is evaluated once at startup to avoid repeated os.Getenv calls | |
| // on every upload request. Set DEVBITS_TRUST_PROXY=true only when the backend | |
| // runs behind a trusted reverse proxy (e.g. AWS ALB) that sets X-Forwarded-Proto. | |
| var trustProxy = os.Getenv("DEVBITS_TRUST_PROXY") == "true" | |
| // trustProxyEnabled reads DEVBITS_TRUST_PROXY at request time so that values | |
| // loaded via godotenv in main() are visible. Set DEVBITS_TRUST_PROXY=true only | |
| // when the backend runs behind a trusted reverse proxy (e.g. AWS ALB) that | |
| // sets X-Forwarded-Proto. | |
| func trustProxyEnabled() bool { | |
| return os.Getenv("DEVBITS_TRUST_PROXY") == "true" | |
| } |
* AWS-ready * Update backend/api/static/account-deletion.html Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix review comments: AASA content-type, proxy header spoofing, API_BASE_URL const mutation, installer guard, docs (#143) * Initial plan * Address PR review comments: AASA content-type, proxy header security, template check, API_BASE_URL fix, docs update Co-authored-by: elifouts <116454864+elifouts@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: elifouts <116454864+elifouts@users.noreply.github.com> * Consolidate static/compliance files to single source of truth with sync script (#144) * Initial plan * Add sync-static.sh to prevent static file drift; fix account-deletion.html email Co-authored-by: grillinr <169214325+grillinr@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: grillinr <169214325+grillinr@users.noreply.github.com> * Refactor scripts and components for improved clarity and functionality; update TypeScript configurations and enhance local development experience. * Update deployment instructions and scripts for AWS EC2 native setup; change default user to ec2-user and enhance clarity in documentation. * updated scripts for aws * updated instructions fr * screw this file * fixed --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: grillinr <169214325+grillinr@users.noreply.github.com>
Addresses five unresolved issues flagged in the AWS-ready PR review.
Content-Type: application/jsonfor app-link filesStaticFilewas used forapple-app-site-associationandassetlinks.json— both require explicitContent-Type: application/jsonfor iOS Universal Links and Android App Links validation. Replaced withGEThandlers that set the header explicitly. Also added/.well-known/apple-app-site-associationas an alias required by iOS.X-Forwarded-Protospoofing fixThe header was trusted unconditionally, allowing any direct caller to inject an arbitrary scheme. Now only honored when
DEVBITS_TRUST_PROXY=true(for ALB/reverse-proxy deployments). The flag is evaluated once at package init rather than per-request.API_BASE_URLconst mutation(exports as any).API_BASE_URL = ...on aconstis a no-op with most bundlers. Replaced with agetValidatedBaseUrl()function that validates and returns the correct URL, which is then assigned to the exportedconst.Systemd installer template guard
install-aws-systemd-service.shransedwithout verifying the template existed, producing an opaque failure. Added an explicit[[ ! -f "$TEMPLATE_PATH" ]]guard with a clear error message.DEEP_LINK_SETUP.mdsource-of-truth fixDocs still referenced
frontend/public/for AASA andassetlinks.json. Updated tobackend/api/static/since nginx is removed and the Go backend serves these files directly.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.