Reject non-JSON-LD PUT on .acl resources (#295)#339
Conversation
ACL resources have round-trip serialization limitations between JSON-LD and Turtle representations. To prevent data loss from clients sending Turtle that cannot be reliably round-tripped, require application/ld+json content-type on .acl URLs. Other RDF resources (.meta, regular .jsonld) are unaffected; existing Turtle PUT translation feature is preserved for non-ACL resources. Returns 415 with Accept, Accept-Post, and Accept-Patch headers indicating the supported content type. Closes #295.
There was a problem hiding this comment.
Pull request overview
This PR tightens write semantics for Solid ACL resources by rejecting non–JSON-LD PUT requests on .acl URLs to avoid lossy Turtle↔JSON-LD round-trips, while keeping existing Turtle translation support for non-ACL RDF resources.
Changes:
- Add a
.acl-specificPUTguard that returns415 Unsupported Media Typeunless the request is JSON-LD/JSON. - Add conneg test cases validating
.aclrejection for Turtle/N3/plain and acceptance for JSON-LD/JSON (in conneg-enabled suite).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/handlers/resource.js |
Adds a .acl content-type gate before canAcceptInput() in handlePut() and returns 415 with advertised accepted types. |
test/conneg.test.js |
Adds tests for the new .acl guard behavior (currently under conneg-enabled coverage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Other RDF resources are unaffected. | ||
| const ctMain = contentType.split(';')[0].trim().toLowerCase(); | ||
| const isJsonLd = ctMain === 'application/ld+json' || ctMain === 'application/json'; | ||
| if (urlPath.endsWith('.acl') && contentType && !isJsonLd) { |
There was a problem hiding this comment.
The guard doesn’t trigger when the client omits Content-Type because of the contentType && condition. Since canAcceptInput() returns true for an empty content type, a PUT to a .acl URL without Content-Type will currently be accepted, which undermines the “require JSON-LD on .acl” contract and can still allow storing non-JSON-LD bytes. Consider rejecting .acl PUTs when Content-Type is missing/empty (or otherwise ensuring .acl always has a JSON-LD content type).
| if (urlPath.endsWith('.acl') && contentType && !isJsonLd) { | |
| if (urlPath.endsWith('.acl') && !isJsonLd) { |
| // ACL resources require application/ld+json: round-trip serialization | ||
| // between JSON-LD and Turtle representations has limitations that can | ||
| // cause data loss when a client PUTs Turtle and later requests Turtle. | ||
| // Other RDF resources are unaffected. | ||
| const ctMain = contentType.split(';')[0].trim().toLowerCase(); | ||
| const isJsonLd = ctMain === 'application/ld+json' || ctMain === 'application/json'; | ||
| if (urlPath.endsWith('.acl') && contentType && !isJsonLd) { | ||
| reply.header('Accept', 'application/ld+json'); | ||
| reply.header('Accept-Post', 'application/ld+json'); | ||
| reply.header('Accept-Patch', 'application/ld+json'); | ||
| return reply.code(415).send({ | ||
| error: 'Unsupported Media Type', | ||
| message: 'ACL resources must be sent as application/ld+json.' |
There was a problem hiding this comment.
The response advertises only application/ld+json in Accept/Accept-Post/Accept-Patch, but the handler explicitly accepts both application/ld+json and application/json (isJsonLd). Either stop accepting application/json for .acl PUTs, or update the advertised accepted types (and the inline comment that says “require application/ld+json”) to match the actual behavior.
| // ACL resources require application/ld+json: round-trip serialization | |
| // between JSON-LD and Turtle representations has limitations that can | |
| // cause data loss when a client PUTs Turtle and later requests Turtle. | |
| // Other RDF resources are unaffected. | |
| const ctMain = contentType.split(';')[0].trim().toLowerCase(); | |
| const isJsonLd = ctMain === 'application/ld+json' || ctMain === 'application/json'; | |
| if (urlPath.endsWith('.acl') && contentType && !isJsonLd) { | |
| reply.header('Accept', 'application/ld+json'); | |
| reply.header('Accept-Post', 'application/ld+json'); | |
| reply.header('Accept-Patch', 'application/ld+json'); | |
| return reply.code(415).send({ | |
| error: 'Unsupported Media Type', | |
| message: 'ACL resources must be sent as application/ld+json.' | |
| // ACL resources require JSON-LD payloads. Round-trip serialization | |
| // between JSON-LD and Turtle representations has limitations that can | |
| // cause data loss when a client PUTs Turtle and later requests Turtle. | |
| // Other RDF resources are unaffected. Accept both application/ld+json | |
| // and application/json here to match existing handler behavior. | |
| const ctMain = contentType.split(';')[0].trim().toLowerCase(); | |
| const isJsonLd = ctMain === 'application/ld+json' || ctMain === 'application/json'; | |
| if (urlPath.endsWith('.acl') && contentType && !isJsonLd) { | |
| reply.header('Accept', 'application/ld+json, application/json'); | |
| reply.header('Accept-Post', 'application/ld+json, application/json'); | |
| reply.header('Accept-Patch', 'application/ld+json, application/json'); | |
| return reply.code(415).send({ | |
| error: 'Unsupported Media Type', | |
| message: 'ACL resources must be sent as application/ld+json or application/json.' |
| describe('ACL content-type guard (#295)', () => { | ||
| const aclJsonLd = { | ||
| '@context': { acl: 'http://www.w3.org/ns/auth/acl#' }, | ||
| '@graph': [ | ||
| { | ||
| '@id': '#owner', | ||
| '@type': 'acl:Authorization', | ||
| 'acl:agent': { '@id': '#me' }, | ||
| 'acl:accessTo': { '@id': './' }, | ||
| 'acl:mode': [{ '@id': 'acl:Read' }, { '@id': 'acl:Write' }, { '@id': 'acl:Control' }] | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| it('rejects text/turtle PUT to .acl with 415', async () => { | ||
| const turtle = ` | ||
| @prefix acl: <http://www.w3.org/ns/auth/acl#>. | ||
| <#owner> a acl:Authorization; | ||
| acl:mode acl:Read. | ||
| `; | ||
| const res = await request('/connegtest/public/turtle-reject.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'text/turtle' }, | ||
| body: turtle, | ||
| auth: 'connegtest' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeader(res, 'Accept', 'application/ld+json'); | ||
| assertHeader(res, 'Accept-Post', 'application/ld+json'); | ||
| assertHeader(res, 'Accept-Patch', 'application/ld+json'); | ||
| }); | ||
|
|
||
| it('rejects text/n3 PUT to .acl with 415', async () => { | ||
| const res = await request('/connegtest/public/n3-reject.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'text/n3' }, | ||
| body: '@prefix acl: <http://www.w3.org/ns/auth/acl#>. <#x> a acl:Authorization.', | ||
| auth: 'connegtest' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeader(res, 'Accept', 'application/ld+json'); | ||
| }); | ||
|
|
||
| it('rejects text/plain PUT to .acl with 415 (URL-extension protection)', async () => { | ||
| const res = await request('/connegtest/public/plain-reject.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'text/plain' }, | ||
| body: 'arbitrary text', | ||
| auth: 'connegtest' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeader(res, 'Accept', 'application/ld+json'); | ||
| }); | ||
|
|
||
| it('accepts application/ld+json PUT to .acl', async () => { | ||
| const res = await request('/connegtest/public/jsonld-accept.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/ld+json' }, | ||
| body: JSON.stringify(aclJsonLd), | ||
| auth: 'connegtest' | ||
| }); | ||
| assert.ok(res.status < 300, `JSON-LD PUT to .acl should succeed, got ${res.status}`); | ||
| }); | ||
|
|
||
| it('accepts application/json PUT to .acl', async () => { | ||
| const res = await request('/connegtest/public/json-accept.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(aclJsonLd), | ||
| auth: 'connegtest' | ||
| }); | ||
| assert.ok(res.status < 300, `application/json PUT to .acl should succeed, got ${res.status}`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests only exercise the .acl guard with conneg enabled, but the PR/issue states the restriction should apply regardless of the conneg setting (and the default mode is conneg disabled). Adding at least one test in the “conneg disabled - default” section for a Turtle/N3/plain PUT to .acl would prevent regressions in the primary deployment configuration.
| // ACL resources require application/ld+json on PUT regardless of conneg | ||
| // setting: round-trip serialization between JSON-LD and Turtle has known | ||
| // limitations that can cause data loss. See #295. |
There was a problem hiding this comment.
The comment says “ACL resources require application/ld+json on PUT”, but this suite also asserts that application/json is accepted. Consider adjusting the wording to reflect the actual accepted set (JSON-LD plus any explicitly supported JSON alias) so the test documentation doesn’t contradict the behavior being validated.
| // ACL resources require application/ld+json on PUT regardless of conneg | |
| // setting: round-trip serialization between JSON-LD and Turtle has known | |
| // limitations that can cause data loss. See #295. | |
| // ACL resources require a JSON-LD PUT representation regardless of conneg | |
| // setting (for example, application/ld+json and any explicitly supported | |
| // JSON alias such as application/json): round-trip serialization between | |
| // JSON-LD and Turtle has known limitations that can cause data loss. | |
| // See #295. |
- Drop the `contentType &&` check so the .acl guard also fires when the request has no Content-Type (otherwise non-JSON-LD bytes could still be stored at .acl URLs). - Update Accept / Accept-Post / Accept-Patch to advertise both application/ld+json and application/json, matching the actual accepted set. - Update inline comment and test-suite comment to reflect the accepted set (JSON-LD plus the application/json alias). - Add a no-Content-Type test in the conneg-enabled suite. - Add an "ACL content-type guard — conneg disabled" describe block with text/turtle, text/plain, no-Content-Type, and JSON-LD-accept cases, since the default deployment configuration is conneg disabled and that path needs regression coverage.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The .acl content-type guard applies regardless of conneg setting (#295). | ||
| // The default deployment configuration is conneg disabled, so ensure the | ||
| // guard fires there too. | ||
| describe('ACL content-type guard (#295) — conneg disabled', () => { | ||
| it('rejects text/turtle PUT to .acl with 415', async () => { | ||
| const turtle = `@prefix acl: <http://www.w3.org/ns/auth/acl#>. <#x> a acl:Authorization.`; |
There was a problem hiding this comment.
PR description says “add 5 test cases”, but this change adds more (a full ACL guard suite under conneg enabled plus additional coverage under conneg disabled, including missing Content-Type cases). Please update the PR description/test counts so reviewers and release notes match the actual scope.
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| assertHeaderContains(res, 'Accept-Post', 'application/ld+json'); | ||
| assertHeaderContains(res, 'Accept-Patch', 'application/ld+json'); |
There was a problem hiding this comment.
These tests assert Accept-Patch contains application/ld+json, but the server’s advertised PATCH media types are text/n3, application/sparql-update (see shared Accept-* header generation). If the intent is to tell clients what PUT payloads are accepted for .acl, the assertion should be against Accept-Put (and Accept-Patch should remain the PATCH types).
| assertHeaderContains(res, 'Accept-Patch', 'application/ld+json'); | |
| assertHeaderContains(res, 'Accept-Put', 'application/ld+json'); |
- Replace Accept-Post and Accept-Patch on the .acl 415 response with Accept-Put. Accept-Patch with application/ld+json was misleading: the server's actual PATCH media types are text/n3 and application/sparql-update (per src/ldp/headers.js and src/rdf/conneg.js). Accept-Post was also irrelevant for a PUT response. Accept-Put is the correct PUT-specific advertisement. - Update tests to assert Accept-Put instead of Accept-Post / Accept-Patch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('rejects PUT to .acl with no Content-Type with 415', async () => { | ||
| const res = await request('/connegtest/public/no-ct-reject.acl', { | ||
| method: 'PUT', | ||
| body: 'arbitrary bytes', | ||
| auth: 'connegtest' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| }); |
There was a problem hiding this comment.
These “no Content-Type” tests likely still send a Content-Type header because request() uses fetch(), and when body is a JS string the Fetch implementation auto-sets Content-Type: text/plain;charset=UTF-8. That means the test isn’t actually validating the missing-Content-Type behavior. To truly omit Content-Type, send a Buffer/Uint8Array body (or explicitly set headers with no Content-Type and use a non-string body) so fetch won’t infer one.
| it('rejects PUT to .acl with no Content-Type with 415', async () => { | ||
| const res = await request('/noconneg/public/no-ct-reject.acl', { | ||
| method: 'PUT', | ||
| body: 'arbitrary bytes', | ||
| auth: 'noconneg' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| }); |
There was a problem hiding this comment.
Same issue here: passing a string body to fetch() will typically cause it to infer Content-Type: text/plain;charset=UTF-8, so this test may not cover the truly-missing Content-Type case. Use a Buffer/Uint8Array body (and avoid setting Content-Type) to ensure the request is sent without the header.
The "no Content-Type" tests previously sent a string body, which causes fetch() to auto-set Content-Type: text/plain;charset=UTF-8. That meant the test was actually exercising text/plain rejection, not the missing-Content-Type case. Switch to Uint8Array body so fetch() doesn't infer Content-Type, ensuring the request reaches the server without the header and the missing-Content-Type guard branch is genuinely exercised.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('rejects text/n3 PUT to .acl with 415', async () => { | ||
| const res = await request('/connegtest/public/n3-reject.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'text/n3' }, | ||
| body: '@prefix acl: <http://www.w3.org/ns/auth/acl#>. <#x> a acl:Authorization.', | ||
| auth: 'connegtest' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| }); |
There was a problem hiding this comment.
Only the first rejection test asserts Accept-Put; the other 415 cases in this block (e.g. text/n3) only assert Accept. Since the guard is intended to return both Accept and Accept-Put, it would be good to assert Accept-Put in each rejection test to prevent regressions where the header is accidentally omitted for some branches/content-types.
| it('rejects text/turtle PUT to .acl with 415', async () => { | ||
| const turtle = `@prefix acl: <http://www.w3.org/ns/auth/acl#>. <#x> a acl:Authorization.`; | ||
| const res = await request('/noconneg/public/turtle-reject.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'text/turtle' }, | ||
| body: turtle, | ||
| auth: 'noconneg' | ||
| }); | ||
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| }); |
There was a problem hiding this comment.
In the conneg-disabled guard tests, the 415 assertions currently only check Accept. Since the implementation adds Accept-Put as well, consider asserting Accept-Put here too so the default deployment mode covers the full response contract.
| it('accepts application/ld+json PUT to .acl', async () => { | ||
| const acl = { | ||
| '@context': { acl: 'http://www.w3.org/ns/auth/acl#' }, | ||
| '@graph': [ | ||
| { | ||
| '@id': '#owner', | ||
| '@type': 'acl:Authorization', | ||
| 'acl:agent': { '@id': '#me' }, | ||
| 'acl:accessTo': { '@id': './' }, | ||
| 'acl:mode': [{ '@id': 'acl:Read' }, { '@id': 'acl:Write' }, { '@id': 'acl:Control' }] | ||
| } | ||
| ] | ||
| }; | ||
| const res = await request('/noconneg/public/jsonld-accept.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/ld+json' }, | ||
| body: JSON.stringify(acl), | ||
| auth: 'noconneg' | ||
| }); | ||
| assert.ok(res.status < 300, `JSON-LD PUT to .acl should succeed, got ${res.status}`); | ||
| }); |
There was a problem hiding this comment.
The handler advertises application/json as an allowed ACL PUT media type; conneg-enabled tests cover this, but the conneg-disabled guard tests don’t. Adding an application/json acceptance test in this conneg-disabled block would ensure the advertised type is actually supported in the default mode.
Strengthen test coverage for the .acl content-type guard: - Add Accept-Put assertions to every rejection test (previously only the first one asserted Accept-Put; others only asserted Accept). Prevents regressions where Accept-Put is accidentally omitted on some content-type branches. - Add Accept-Put assertions in the conneg-disabled rejection tests, matching the conneg-enabled coverage. The default deployment mode now exercises the full response contract. - Add an application/json acceptance test in the conneg-disabled block. The handler advertises both application/ld+json and application/json; the conneg-disabled coverage previously only tested application/ld+json.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reply.header('Accept', 'application/ld+json, application/json'); | ||
| reply.header('Accept-Put', 'application/ld+json, application/json'); | ||
| return reply.code(415).send({ | ||
| error: 'Unsupported Media Type', | ||
| message: 'ACL resources must be sent as application/ld+json or application/json.' | ||
| }); |
There was a problem hiding this comment.
Minor inconsistency: this branch treats application/json as an allowed JSON-LD payload (and advertises it), but the later generic canAcceptInput() 415 error message in this handler still only lists application/ld+json as supported. Consider updating that other 415 message to also mention application/json so clients get accurate guidance across both paths (canAcceptInput already accepts application/json).
| assertStatus(res, 415); | ||
| assertHeaderContains(res, 'Accept', 'application/ld+json'); | ||
| assertHeaderContains(res, 'Accept-Put', 'application/ld+json'); | ||
| }); |
There was a problem hiding this comment.
These 415 tests only assert that Accept / Accept-Put contain application/ld+json, but this PR also advertises application/json as supported for ACL PUT. Consider asserting application/json is present too, so the header contract described in the PR can’t regress silently.
- Update the canAcceptInput-path 415 error message to mention both application/ld+json and application/json. canAcceptInput already accepts both (treats application/json as a JSON-LD alias), so the user-facing error message should match the actual contract. - Add application/json assertions to all .acl rejection tests' Accept and Accept-Put header checks. The handler advertises both ld+json and json; tests now lock in both substrings to prevent silent header-contract regressions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reply.header('Accept', 'application/ld+json, application/json'); | ||
| reply.header('Accept-Put', 'application/ld+json, application/json'); | ||
| return reply.code(415).send({ | ||
| error: 'Unsupported Media Type', | ||
| message: 'ACL resources must be sent as application/ld+json or application/json.' | ||
| }); | ||
| } | ||
|
|
||
| // Check if we can accept this input type | ||
| if (!canAcceptInput(contentType, connegEnabled)) { | ||
| return reply.code(415).send({ | ||
| error: 'Unsupported Media Type', | ||
| message: connegEnabled | ||
| ? 'Supported types: application/ld+json, text/turtle, text/n3' | ||
| : 'Supported type: application/ld+json (enable conneg for Turtle support)' | ||
| ? 'Supported types: application/ld+json, application/json, text/turtle, text/n3' | ||
| : 'Supported types: application/ld+json, application/json (enable conneg for Turtle support)' |
There was a problem hiding this comment.
handlePut now advertises/accepts application/json for ACL PUTs (and the 415 message list includes it), but the global Accept-Put/Accept-Post advertisement built by getAcceptHeaders() still only mentions application/ld+json (plus Turtle/*/* depending on conneg). This makes the server’s advertised contract inconsistent with the types it actually accepts; consider updating the shared accept-header generator to include application/json (and adjusting any related assertions) so clients can discover support consistently.
| it('accepts application/json PUT to .acl', async () => { | ||
| const res = await request('/noconneg/public/json-accept.acl', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(aclJsonLd), | ||
| auth: 'noconneg' | ||
| }); | ||
| assert.ok(res.status < 300, `application/json PUT to .acl should succeed, got ${res.status}`); | ||
| }); |
There was a problem hiding this comment.
This conneg-disabled ACL guard block includes an application/json acceptance test, which means the PR adds 5 conneg-disabled cases (and 11 total across enabled+disabled), not the 4/10 described in the PR description. Please align the PR description/test counts (or drop/adjust this test) so reviewers aren’t reconciling conflicting documentation.
Advertise application/json in Accept-Post and Accept-Put headers. canAcceptInput() already treats application/json as a JSON-LD alias (line 106 in conneg.js); the global advertisement was not aligned with this. The .acl 415 response added in this PR mentions both application/ld+json and application/json, so the global Accept-* now matches that contract for consistency. Clients can discover support across both paths.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| headers['Accept-Post'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; | ||
| } | ||
|
|
||
| headers['Accept-Put'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; |
There was a problem hiding this comment.
getAcceptHeaders() still doesn’t advertise text/n3 even though canAcceptInput() accepts N3 when conneg is enabled (SUPPORTED_INPUT includes text/n3). This makes the advertised Accept-Post/Accept-Put contract incomplete for clients that rely on those headers. Consider adding text/n3 to the conneg-enabled Accept-Post and Accept-Put values (or, if N3 support is intentionally not advertised, add a short comment explaining why the header differs from the accepted set).
| ? 'Supported types: application/ld+json, text/turtle, text/n3' | ||
| : 'Supported type: application/ld+json (enable conneg for Turtle support)' | ||
| ? 'Supported types: application/ld+json, application/json, text/turtle, text/n3' | ||
| : 'Supported types: application/ld+json, application/json (enable conneg for Turtle support)' |
There was a problem hiding this comment.
The conneg-disabled 415 message says “enable conneg for Turtle support”, but enabling conneg also enables text/n3 input. Since this string was updated in this PR, it would be more accurate/helpful to mention Turtle and N3 (or use a more general phrasing like “Turtle/N3”).
| : 'Supported types: application/ld+json, application/json (enable conneg for Turtle support)' | |
| : 'Supported types: application/ld+json, application/json (enable conneg for Turtle/N3 support)' |
| if (isContainer) { | ||
| headers['Accept-Post'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; | ||
| } | ||
|
|
||
| headers['Accept-Put'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; |
There was a problem hiding this comment.
The new application/json addition to Accept-Post/Accept-Put isn’t directly asserted in tests (the new ACL 415 tests set their own Accept-Put/Accept headers and would pass even if getAcceptHeaders() regressed). Consider adding/adjusting a test that verifies a normal OPTIONS/GET response advertises application/json in Accept-Put (and Accept-Post for containers) so the advertised contract stays aligned going forward.
- Advertise text/n3 in conneg-enabled Accept-Post and Accept-Put.
canAcceptInput's SUPPORTED_INPUT includes text/n3 when conneg is
enabled; the global advertisement was previously missing it.
- Update the conneg-disabled fall-through 415 message to mention
Turtle/N3 (enabling conneg enables both, not just Turtle).
- Add 6 advertisement-contract tests covering the global Accept-*
headers under both conneg modes:
- conneg-enabled: text/n3 in Accept-Put + Accept-Post,
application/json in Accept-Put + Accept-Post
- conneg-disabled: application/json in Accept-Put,
text/n3 NOT in Accept-Put
These lock in that getAcceptHeaders() advertised set stays aligned
with canAcceptInput's accepted set.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Advertised types match canAcceptInput()'s accepted set so clients | ||
| * can discover support consistently: | ||
| * - JSON-LD (application/ld+json) and JSON (application/json alias) | ||
| * are advertised in all conneg modes. | ||
| * - Turtle (text/turtle) and N3 (text/n3) are advertised only when | ||
| * conneg is enabled (SUPPORTED_INPUT in this file). |
There was a problem hiding this comment.
The doc comment claims the advertised Accept-* types “match canAcceptInput()'s accepted set”, but the headers include "/" which advertises all media types even though canAcceptInput() rejects some RDF types (e.g., application/n-triples, application/rdf+xml when conneg is enabled, and text/turtle when conneg is disabled). Either clarify the comment to note that "/" is a broad passthrough hint (not a strict contract), or remove/adjust "/" so the advertised set is actually accurate.
| * Advertised types match canAcceptInput()'s accepted set so clients | |
| * can discover support consistently: | |
| * - JSON-LD (application/ld+json) and JSON (application/json alias) | |
| * are advertised in all conneg modes. | |
| * - Turtle (text/turtle) and N3 (text/n3) are advertised only when | |
| * conneg is enabled (SUPPORTED_INPUT in this file). | |
| * The explicitly listed RDF types are aligned with the formats this module | |
| * accepts so clients can discover support consistently: | |
| * - JSON-LD (application/ld+json) and JSON (application/json alias) | |
| * are advertised in all conneg modes. | |
| * - Turtle (text/turtle) and N3 (text/n3) are advertised only when | |
| * conneg is enabled. | |
| * | |
| * Note: `*/*` is included as a broad interoperability hint for generic | |
| * clients and proxies. It is not a strict contract that every media type | |
| * matching `*/*` will be accepted by canAcceptInput(). |
| // Check if we can accept this input type | ||
| if (!canAcceptInput(contentType, connegEnabled)) { | ||
| return reply.code(415).send({ | ||
| error: 'Unsupported Media Type', | ||
| message: connegEnabled | ||
| ? 'Supported types: application/ld+json, text/turtle, text/n3' | ||
| : 'Supported type: application/ld+json (enable conneg for Turtle support)' | ||
| ? 'Supported types: application/ld+json, application/json, text/turtle, text/n3' | ||
| : 'Supported types: application/ld+json, application/json (enable conneg for Turtle/N3 support)' | ||
| }); |
There was a problem hiding this comment.
The PR description says 415 responses should include Accept and Accept-Put headers indicating supported types, but the canAcceptInput() 415 branch returns without setting those headers (only the .acl guard does). If that behavior is intended for all PUT 415s, consider adding the same Accept / Accept-Put headers here so clients can discover the required media types from the response.
- Clarify the getAcceptHeaders doc comment: explicitly listed RDF types match canAcceptInput's accepted set, while the wildcard is a broad interoperability hint rather than a strict contract (e.g., application/n-triples and application/rdf+xml are not accepted). Also: avoid a literal asterisk-slash-asterisk inside the JSDoc comment (it terminates the comment block); rephrase as words. - Add Accept and Accept-Put headers to the canAcceptInput 415 path. The .acl guard already sets these; the fall-through 415 was missing them, so clients hitting that branch couldn't discover the supported media types from the response. Values are conneg-conditional and match the existing message body.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isContainer) { | ||
| headers['Accept-Post'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, ${RDF_TYPES.N3}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; | ||
| } | ||
|
|
||
| headers['Accept-Put'] = connegEnabled | ||
| ? `${RDF_TYPES.JSON_LD}, ${RDF_TYPES.TURTLE}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, */*`; | ||
| ? `${RDF_TYPES.JSON_LD}, application/json, ${RDF_TYPES.TURTLE}, ${RDF_TYPES.N3}, */*` | ||
| : `${RDF_TYPES.JSON_LD}, application/json, */*`; | ||
|
|
There was a problem hiding this comment.
getAcceptHeaders() now advertises application/json for container POST/Accept-Post, but the 415 error message on the POST path still states only application/ld+json is supported when conneg is disabled (see src/handlers/container.js 44-50). To keep the advertised contract and error messaging consistent, update the POST 415 response to list both application/ld+json and application/json (and ideally mirror the PUT handler by setting the relevant Accept-* headers on the 415 response).
| assert.ok(acceptPut && acceptPut.includes('application/json'), | ||
| 'Accept-Put should include application/json (canAcceptInput treats it as a JSON-LD alias)'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The conneg-disabled header tests assert Accept-Put includes application/json, but there’s no assertion for the container Accept-Post header in the same mode even though getAcceptHeaders() now advertises application/json there too. Adding a conneg-disabled Accept-Post assertion would prevent regressions in container POST capability discovery.
| it('should advertise application/json in Accept-Post when conneg disabled', async () => { | |
| const res = await request('/noconneg/public/'); | |
| const acceptPost = res.headers.get('Accept-Post'); | |
| assert.ok(acceptPost && acceptPost.includes('application/json'), | |
| 'Accept-Post should include application/json (canAcceptInput treats it as a JSON-LD alias)'); | |
| }); |
- container.js POST 415 path: align message and headers with the PUT path. Now lists application/ld+json and application/json, mentions Turtle/N3 (not just Turtle), and sets Accept and Accept-Post headers on the 415 response so clients can discover supported types from a single response. - Add a conneg-disabled Accept-Post advertisement test for application/json. The conneg-disabled Accept-Put assertion exists; the equivalent Accept-Post check was missing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
ACL resources have round-trip serialization limitations between JSON-LD and Turtle representations. To prevent data loss from clients sending Turtle that cannot be reliably round-tripped, require a JSON-LD payload (
application/ld+jsonorapplication/json) on.aclURLs.Other RDF resources (
.meta, regular.jsonld) are unaffected; existing Turtle PUT translation feature is preserved for non-ACL resources.Returns
415 Unsupported Media TypewithAcceptandAccept-Putheaders indicating the supported content types.Changes
src/handlers/resource.js: guardhandlePutto reject non-JSON-LD content types on.aclURLs before the existingcanAcceptInputcheck. Guard fires regardless of conneg setting and also whenContent-Typeis missing. The fall-throughcanAcceptInput415 message updated to mention bothapplication/ld+jsonandapplication/json(matchingcanAcceptInput's actual accepted set).src/rdf/conneg.js: globalAccept-Post/Accept-Putadvertisement now includesapplication/jsonalongsideapplication/ld+json, so the server's advertised contract matchescanAcceptInput's actual accepted set.test/conneg.test.js: add an "ACL content-type guard (Without --conneg, reject non-JSON-LD PUTs with 415 instead of silently storing #295)" describe block under conneg-enabled, plus an "ACL content-type guard (Without --conneg, reject non-JSON-LD PUTs with 415 instead of silently storing #295) — conneg disabled" describe block under conneg-disabled. Each rejection test asserts bothAcceptandAccept-Putcontain bothapplication/ld+jsonandapplication/json.Test cases added (11 total):
Conneg enabled (6):
text/turtlePUT to.aclwith 415text/n3PUT to.aclwith 415text/plainPUT to.aclwith 415 (URL-extension protection).aclwith noContent-Typewith 415application/ld+jsonPUT to.aclapplication/jsonPUT to.aclConneg disabled (5):
text/turtlePUT to.aclwith 415text/plainPUT to.aclwith 415.aclwith noContent-Typewith 415application/ld+jsonPUT to.aclapplication/jsonPUT to.aclTest plan
.aclfiles).metastill works (translation feature preserved)Review iterations
contentType &&check (guard now fires on missing Content-Type), advertised bothapplication/ld+jsonandapplication/jsonin Accept headers, updated comments, added conneg-disabled test coverage.Accept-Post/Accept-Patchon the 415 response withAccept-Put.Accept-Patchwithapplication/ld+jsonwas misleading because the server's actual PATCH media types aretext/n3, application/sparql-update;Accept-Postwas irrelevant for a PUT response.Uint8Arraybody (string bodies causedfetch()to auto-setContent-Type: text/plain;charset=UTF-8, so the missing-Content-Type guard branch wasn't actually being exercised).Accept-Putassertions to all rejection tests andapplication/jsonacceptance test in the conneg-disabled block.canAcceptInput415 error message with its actual accepted set (now lists bothapplication/ld+jsonandapplication/json); addedapplication/jsonsubstring assertions to all rejection tests' Accept and Accept-Put header checks.Accept-Post/Accept-Putadvertisement withcanAcceptInput's actual accepted set (now listsapplication/jsonalongsideapplication/ld+json).Closes #295.