fix(api): stop leaking destination errors, honour the mapped prefix, guard schedule names - #261
Merged
Merged
Conversation
…guard schedule names
Four API-layer defects recorded in docs/STATUS-AND-BACKLOG.md §6, each with a single
defensible answer, so none needed a product decision.
- `GET /jobs/{id}` and `GET /jobs/{id}/events` returned a destination's own failure text
verbatim. `S3Destination` interpolates `s3://{bucket}/{key}` plus the AWS SDK's message
and `LocalDestination` an `IOException` carrying the full server path, so both routes
handed infrastructure detail to any API caller. The runner now persists and emits only
the file name and destination type and keeps the reason in the log, matching what the
read-failure path beside it already does. Scrubbing at the runner rather than in each
destination also covers third-party `IDestination` implementations, and `UploadResult`
itself is untouched (`Abstractions` is frozen).
- `Location` was built from a hardcoded `/api`, so under `MapNeoReports("/v2")` the 202's
header was a 404 for any client that followed it. A group-level endpoint filter carries
the mapped prefix on the request and the three `Created`/`Accepted` sites build from it.
- `PUT`/`DELETE /reports/{name}/schedule` reached a name-validating override store
unguarded, so a legitimate code-first report named `sales.daily` got an
`ArgumentException` 500. Both now answer 409 naming the pattern, like the
"this host cannot do that" response already next to them. The read path guarded
already, which is what made this an oversight.
- `POST`/`PUT /sources` stored the caller's property bag unnormalized, so under
`AddInMemorySourceRegistry()` a source created over HTTP failed later with "requires a
non-empty 'connectionString' property" — the value was a `JsonElement`. Both handlers
now reuse the run endpoint's normalizer, renamed `NormalizeJsonValues` as it serves two
request shapes.
Seven tests, each verified to fail against the unfixed code. The `Location` test follows
the header instead of string-matching it, so a well-formed-but-wrong URL still fails, and
the upload assertions cover the events feed as well as the job's error.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
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
2 participants
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.
Four API-layer defects from
docs/STATUS-AND-BACKLOG.md§6. They were deferred as a group, but unlike the rest of that list each has a single defensible answer, so none of them needed a product decision — the remaining §6 items (page-loop safety net, Elasticsearch partial failure, page-size clamps) still do and are untouched here.Destination failure text reached API callers
GET /jobs/{id}andGET /jobs/{id}/eventsreturned a destination's own message verbatim.S3Destinationinterpolatess3://{bucket}/{key}plus the AWS SDK's text;LocalDestinationanIOExceptioncarrying the full server path. Both routes therefore handed infrastructure detail to any caller, while the sync endpoint deliberately suppressed the same string — one route hid what the other published.The runner now persists (and emits) only the file name and destination type, keeping the reason in the log. That mirrors the read-failure path a few lines above, which already reduces a non-NeoReports exception to its type name. Scrubbing at the runner rather than in each destination is also what covers third-party
IDestinationimplementations, and it leavesUploadResultalone —Abstractionsis frozen.Locationignored the configured prefixBuilt from a hardcoded
/api, so underMapNeoReports("/v2")the 202'sLocationwas a 404 for any client that followed it. The handlers are static method groups with no closure overprefix, so a group-level endpoint filter carries the mapped prefix on the request and the threeCreated/Acceptedsites build their URL from it.Schedule writes hit a name-validating store unguarded
A code-first report is under no naming constraint, but an override store keys by name and only accepts
DynamicReportName.Pattern. A perfectly legalsales.dailyproduced anArgumentException→ 500. Both write paths now answer 409 naming the pattern, consistent with the "this host cannot do that" response already beside them. The read path guarded already, which is what makes this an oversight rather than a design.Source property bags were not normalized
SourceRequest.Propertiesis the same caller-suppliedobject?bag as run parameters.FileSourceRegistryStorelaunders it,InMemorySourceRegistryStoredoes not — so withAddInMemorySourceRegistry()a source created over HTTP failed later with "requires a non-empty 'connectionString' property", the value being aJsonElement. Both handlers now reuse the run endpoint's normalizer, renamedNormalizeJsonValuessince it serves two request shapes.Tests
Seven added, each verified to fail against the unfixed code (production changes stashed, tests kept).
Locationtest follows the header under a/v2mapping rather than string-matching it, so a well-formed-but-wrong URL still fails.Errorcontained "Access Denied" — it encoded the leak, and is inverted here.dotnet test: Core 300/300, AspNetCore 155/155, Jobs 34/34, Destinations.Local 14/14, Destinations.S3 3/3 — 0 failures.