hotfix: Block generate signature endpoint #102
Conversation
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing touches🧪 Generate unit tests
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 |
gocanto
changed the title
![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
🧹 Nitpick comments (7)
.gitignore (2)
8-11: Broaden ignore to catch files without extensions.Patterns
*.*miss files likeREADME,index, or extension-less certs. Prefer directory-scoped ignores while keeping.gitkeep.Apply:
-# --- [Caddy]: mtls -caddy/mtls/*.* -!caddy/mtls/.gitkeep +# --- [Caddy]: mtls +caddy/mtls/** +!caddy/mtls/.gitkeep
19-26: Same for storage: avoid*.*to ensure all artifacts are ignored.
s torage/logs/caddy/*.*and related rules can leak files without extensions.Apply:
-storage/logs/*.* -storage/media/*.* -storage/logs/caddy/*.* -storage/media/posts/*.* -storage/media/users/*.* +storage/logs/** +storage/media/** +!storage/logs/.gitkeep +!storage/logs/caddy/.gitkeep +!storage/media/.gitkeep +!storage/media/posts/.gitkeep +!storage/media/users/.gitkeepmetal/makefile/app.mk (1)
1-1: Stale PHONY entry for validate-caddy.You removed/relocated the old target but
.PHONYstill listsvalidate-caddy. Either drop it or create an alias to the new target.Apply one of:
-.PHONY: fresh destroy audit watch format run-cli validate-caddy test-all run-cli-local +.PHONY: fresh destroy audit watch format run-cli test-all run-cli-localor
+.PHONY: validate-caddy +validate-caddy: caddy-validateMakefile (1)
100-103: Helpful help entries; minor wording/consistency.Consider consistent naming (“Caddy mTLS certificates”) and aligning with any alias
validate-caddyif you add it.Apply:
- $(BOLD)$(GREEN)caddy-gen-cert$(NC) : Generate the caddy's mtls certificates. - $(BOLD)$(GREEN)caddy-validate$(NC) : Validates caddy's files syntax. + $(BOLD)$(GREEN)caddy-gen-cert$(NC) : Generate Caddy mTLS CA (ca.key/ca.pem). + $(BOLD)$(GREEN)caddy-validate$(NC) : Validate Caddyfile syntax (prod/local).caddy/Caddyfile.prod (1)
28-33: Public listener block rule: good placement and intent.Matcher sits before
/api/*handler; 403 is returned early. Consider 404 to reduce endpoint discoverability if that matters.Possible tweak:
-respond 403 +respond 404metal/makefile/caddy.mk (2)
13-19: Developer ergonomics: add a Docker-based validator to avoid local Caddy dependency.Keep the current target; add a dockerized variant to work anywhere.
Add:
.PHONY: caddy-validate-docker caddy-validate-docker: docker run --rm -v "$(ROOT_PATH)/caddy:/etc/caddy" caddy:2.10.0 \ sh -lc 'caddy fmt --overwrite /etc/caddy/Caddyfile.prod && caddy validate --config /etc/caddy/Caddyfile.prod && caddy fmt --overwrite /etc/caddy/Caddyfile.local && caddy validate --config /etc/caddy/Caddyfile.local'
1-6: Optional: provide a client cert generator.You require-and-verify client certs; a helper target reduces footguns (sets EKU=clientAuth, SAN, validity).
Add:
.PHONY: caddy-gen-client-cert caddy-gen-client-cert: @[ -f $(CADDY_MTLS_DIR)/ca.key ] || (echo "Missing CA; run make caddy-gen-cert" && exit 1) openssl genrsa -out $(CADDY_MTLS_DIR)/client.key 2048 openssl req -new -key $(CADDY_MTLS_DIR)/client.key -subj "/CN=oullin-mtls-client" -out $(CADDY_MTLS_DIR)/client.csr openssl x509 -req -in $(CADDY_MTLS_DIR)/client.csr -CA $(CADDY_MTLS_DIR)/ca.pem -CAkey $(CADDY_MTLS_DIR)/ca.key -CAcreateserial \ -out $(CADDY_MTLS_DIR)/client.crt -days 825 -sha256 \ -extfile <(printf "basicConstraints=CA:FALSE\nkeyUsage=critical,digitalSignature,keyEncipherment\nextendedKeyUsage=clientAuth\nsubjectAltName=DNS:client") rm -f $(CADDY_MTLS_DIR)/client.csr chmod 600 $(CADDY_MTLS_DIR)/client.key chmod 644 $(CADDY_MTLS_DIR)/client.crt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)Makefile(2 hunks)caddy/Caddyfile.prod(2 hunks)docker-compose.yml(1 hunks)metal/makefile/app.mk(1 hunks)metal/makefile/caddy.mk(1 hunks)
🔇 Additional comments (3)
metal/makefile/app.mk (1)
58-58: No actionable change in this hunk.Nothing to address in this specific line.
Makefile (1)
43-43: Including caddy.mk: LGTM.Centralizing Caddy targets/vars is cleaner.
caddy/Caddyfile.prod (1)
90-112: Internal mTLS block: path stripping and TLS syntax.
- handle_path /api/generate-signature* strips the entire matched prefix (/api/generate-signature) before proxying; if the upstream expects /generate-signature, strip only /api (use a named matcher +
uri strip_prefix /apiorhandle /api/*withuri strip_prefix /api). (caddyserver.com)- TLS syntax:
tls internal { client_auth { ... } }is valid in Caddy 2.10.0; usetls { issuer internal { ... } }only when you need to configure internal-CA options (e.g., custom CA name/lifetime). No mandatory rewrite of the TLS form is required just to nestclient_auth. (caddyserver.com)- reverse_proxy: add transport timeouts (e.g.,
dial_timeout,response_header_timeout) to protect against slow/unresponsive upstreams. (scwy.net)Validation step not completed here (docker not available:
/bin/bash: line 3: docker: command not found). Run locally and paste output:
docker run --rm -v "$PWD/caddy:/etc/caddy" caddy:2.10.0 caddy validate --config /etc/caddy/Caddyfile.prod
Summary by CodeRabbit
New Features
Chores