Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several authentication, form-parsing, and CSP behaviors are documented inaccurately.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the missing client-focused API reference for wakeci.
Changes:
- Documents REST, authentication, storage, and static routes.
- Defines WebSocket protocol behavior and API conventions.
- Records current implementation quirks and response semantics.
File summaries
| File | Description |
|---|---|
API.md |
Adds the complete HTTP and WebSocket API reference. |
Review details
Suppressed comments (2)
API.md:219
- A blocked client does not necessarily receive
429here. With a valid session cookie,AuthMibypasses the password limiter and this probe still returns200;429occurs when the probe takes the Basic-auth path.
Lightweight authentication probe. Returns `200 OK` with an empty body when the
request is authenticated, `403 Forbidden` with the body `Forbidden` when it is
not, or `429 Too Many Requests` with the body `Too many authentication attempts`
when the client is blocked by the rate limiter.
API.md:746
- This CSP does not prevent every credentialed callback into the API. Because
default-src 'self'still permits same-origin subresources and navigation is unrestricted, an artifact can issue authenticated GET requests (for example through an image) or navigate to/auth/logout; only connect APIs and form submission are blocked. Avoid presenting the policy as a complete request boundary.
**Response headers.** `/storage/build/*` gets a Content-Security-Policy of its
own — loose enough to preview HTML artifacts, locked down so a previewed
artifact can never use the caller's credentials to call back into the API:
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AsLY4
force-pushed
the
api-documentation
branch
from
September 2, 2026 23:01
0a8af75 to
b1a76eb
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several documented behaviors are inaccurate, including a security-critical claim about artifact isolation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 4
- Review effort level: Balanced
The README already links to `API.md` at the repository root, but the file was
never added. This is that reference, written against the current code.
* REST API under `/api` and the `/auth` helpers: every endpoint's inputs,
status codes and response bodies, including the ones that are easy to get
wrong — `GET /api/build/{id}` answers `500` (not `404`) for an id that was
never used, `POST /api/build/{id}/flush` answers `404` for a build that is
merely queued, and `POST /api/settings` applies its fields one at a time and
returns on the first *reported* failure, so a `500` does not mean nothing
changed — while `concurrentBuilds` is the one field whose database error is
never reported at all, so a `200` is not proof that it was saved.
* Authentication: HTTP Basic and the `session` cookie with its exact
attributes and the conditions under which `Secure` is set, plus the
failed-authentication rate limiter (`429`, `Retry-After`, `X-RateLimit-*`).
The limiter is consulted only on the Basic-auth branch, so a blocked client
holding a valid session cookie is still served.
* WebSocket API at `/ws`: the same-origin check on the upgrade, the message
envelope — several messages may share one frame, separated by `\n` — the
anchored subscription matching (only a subscription ending in `:` matches by
prefix), the keep-alive, size and buffer limits, and which malformed
messages end the connection versus which are ignored. A `data` of `null`,
`{}` or `{"to": null}` is not an error: it subscribes to nothing, silently.
* Storage under `/storage/build/`: the actual URL shapes for task logs and
artifacts, directory listings, and the dedicated Content-Security-Policy.
That policy binds the artifact's own document, not the origin — everything
wakeci serves is one origin, `frame-ancestors 'self'` is set everywhere and
there is no CSRF token — so the document says plainly that it is not a
security boundary and that an artifact must be treated as untrusted code
running with the viewer's session.
* Conventions: exact path matching, `null` vs `[]` on empty collections, build
vs task statuses, task kinds and their execution order, the units of
`duration` and `eta`, when a final status can still carry `duration: 0`, and
the one endpoint that ignores a multipart body (`POST /api/job/{name}/run`
parses the request itself and reads `r.Form`, where every other input goes
through `FormValue`).
Two notes on units and generated docs, from reading the code rather than the
comments: `eta` is expressed in nanoseconds like `duration` (the `// seconds`
comment on the struct field is stale, and the frontend divides by 10^9), and
`POST /api/build/{id}/start` is absent from the generated Swagger spec because
its handler carries the `/build/{id}/abort` route annotation.
AsLY4
force-pushed
the
api-documentation
branch
from
September 3, 2026 20:18
b1a76eb to
f85cf8d
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several documented behaviors are inaccurate regarding session persistence, interrupted-build timestamps, and bcrypt password limits.
Review effort: Balanced
Findings: 3
Open (3)
Resolved since last review (4)
default-src 'self'is the fallback forframe-src, and API…concurrentBuildsdo not produce the documented500:…data: null,{}, or{"to": null}successfully unmarshal and…on_pending, butStartedAtis assigned only when the build…
Comment on lines
+42
to
+49
| 2. **Session cookie** — obtain a `session` cookie from `POST /auth/login` and | ||
| send it back on subsequent requests. This is what the web frontend uses. The | ||
| cookie is `HttpOnly`, `SameSite=Strict`, `Path=/` and valid for 120 hours | ||
| (5 days). It additionally carries `Secure` whenever the request looks like | ||
| HTTPS — the server is configured with `port: 443`, the connection is TLS, or | ||
| the first value of `X-Forwarded-Proto` is `https` (case-insensitive). A | ||
| TLS-terminating reverse proxy must forward that header, otherwise the cookie | ||
| is issued without `Secure`. |
Comment on lines
+139
to
+145
| - A final status is not a promise of a real duration. A build aborted through | ||
| `POST /api/build/{id}/abort` while it was still queued never started and ends | ||
| with the saturated `duration: 9223372036854775807` beside the zero | ||
| `startedAt`; a build left `pending` or `running` by a server restart keeps | ||
| `duration: 0` permanently once `GET /api/feed` rewrites its status to | ||
| `aborted`. Detect "never ran" from the zero `startedAt`, which both cases | ||
| share — not from the saturated value. |
Comment on lines
+684
to
+687
| - `password` — `string`, optional. When non-empty, sets a new password (stored | ||
| bcrypt-hashed). Any non-empty value is accepted — there is no length or | ||
| strength check — and an empty value is ignored, so the password cannot be | ||
| cleared this way. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The README links to
API.mdat the repository root, but the file was never added — the link is currently dead. This is that reference, written againstmasteras of 6c6223d.It covers the REST API under
/apiand/auth, the WebSocket API at/ws, the storage endpoint under/storage/build/, the unauthenticated static and/docsroutes, and a conventions section (path matching,nullvs[], build vs task statuses, task kinds and ordering, units).The emphasis is on what a client actually observes and on the behaviour that is easy to get wrong, for example:
GET /api/build/{id}answers500— not404— for an id that was never used, because the build plan is read from disk before the history is queried; the404is reserved for a build whose files exist but whose history entry is gone.POST /api/build/{id}/flushanswers404for a build that is merely queued, and bothabortandflushare best effort: the signal is dropped when no task command is executing, and the response is still200.POST /api/settingsapplies its fields one at a time and returns on the first failure, so a500does not mean nothing changed — posting onlypassword=stores the new password and then fails on the missingconcurrentBuilds.params,artifactsandbuild_artifactsarenullrather than[], and so is the whole body of/api/feedand/api/jobs/when there is nothing to return.:matches by prefix.\n.429withRetry-AfterandX-RateLimit-*) applies toPOST /auth/loginand to Basic auth, and blocks even a request carrying the correct password.Two observations about the code, not addressed here
While documenting units and the generated spec I ran into two things that look like small bugs. I have left the code alone and documented the current behaviour; happy to open separate PRs if you want them fixed:
etais in nanoseconds, not seconds.SetBuildStatusrecordsRecordBuildDuration(b.Job.Name, int(b.Duration))— atime.Duration— andGetJobETA/calcAvgjust average those integers. The// secondscomment onBuild.ETAis stale, and the frontend confirms the real unit by dividing by10**9inSimpleDuration.vue.POST /api/build/{id}/startis missing from the generated Swagger spec.HandleStartBuildcarries a copy-pasted@Router /build/{id}/abort [post]annotation, so the emitted spec has no/build/{id}/startpath. A few other annotations also spell paths with a trailing slash the router rejects (/settings/,/job/{name}/).How it was checked
Written by reading the handlers, middleware, queue, build and WebSocket sources, cross-checked against the existing
_test.gofiles and the frontend's own use of the API. A few serialisation details that are hard to be sure of by reading — nil slice marshalling tonull, the zero-time rendering, the saturated duration of a build aborted while queued, the exactfiltermatch string — were confirmed by running equivalent code in agolang:1.25container. No source file is modified by this PR.