fix: slugs ending on slash with link header cannot create containers #1171

Falx · 2022-02-17T10:56:30Z

📁 Related issues

✍️ Description

This PR makes creating containers by POSTing a slug ending on / illegal if there is no Link header present that declares it as a Container type.

While at first this seems the logical place to change this behaviour:
https://github.com/solid/community-server/blob/d2870e5c8bc2d86b1a43db3a8e039f8b494657da/src/storage/DataAccessorBasedStore.ts#L533-L539

Because of the special root container / case, it was cleaner and less code to just make the change here (in the createSafeUri(...) method):
https://github.com/solid/community-server/blob/44d43456b168fc5e692e7ae1fcbe85d4beeb61b0/src/storage/DataAccessorBasedStore.ts#L506-L510

I've labeled this with semver.major because I think it will break some peoples assumptions on how to create containers.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

this PR is labeled with the correct semver label
- semver.patch: Backwards compatible bug fixes.
- semver.minor: Backwards compatible feature additions.
- semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
the RELEASE_NOTES.md document in case of relevant feature or config changes.

Falx · 2022-02-17T11:00:00Z

(Force pushed to rebase onto versions/3.0.0 branch)

joachimvh · 2022-02-17T12:15:18Z

src/storage/DataAccessorBasedStore.ts

    const isContainer = this.isNewContainer(metadata);
    const slug = metadata.get(SOLID_HTTP.slug)?.value;
+    // Error if slug ends on slash, but no ContainerType Link header is present
+    if (slug && isContainerPath(slug) && !this.hasContainerType(metadata.getAll(RDF.type))) {
+      throw new BadRequestHttpError(`Slugs cannot end with '/' without an HTTP Link header with rel="type" targeting ` +
+      `a valid LDP container type.`);
+    }
    metadata.removeAll(SOLID_HTTP.slug);

    let newID: ResourceIdentifier = this.createURI(container, isContainer, slug);


The problem with this solution is that the isNewContainer is going to return an incorrect result since that still checks the slug. That function should be updated to only check the link type. Additionally, the createUri function calls cleanSlug which also throws an error if there is a slash so now there's some duplication there.

My suggestion would be to instead fix the isNewContainer function, merge createUri with cleanSlug, and update the error there so it only gets thrown in case the result is not a container.

Ok, but cleanSlug() actually does more: it checks that the slug path does not contain any slashes. So in case of a directory (read: Link header is ok) a slug like my/test/slug should still error with the specific Slugs should not contain slashes error.

While myTestSlug/ without a correct Link header could error with Slugs cannot end with '/' without an HTTP Link header with rel="type" targeting a valid LDP container type.

So I would:

Fix isNewContainer() like you said: only check on the link header being present and correct

keep cleanSlug() as is

Revert createSafeUri()

Modify createURI(): test for both cases there:

isContainer == true AND slug.endswitch('/') => throw my error

cleanSlug() call: throws if slashes are still inside slug with existing error there

Ok, but cleanSlug() actually does more: it checks that the slug path does not contain any slashes. So in case of a directory (read: Link header is ok) a slug like my/test/slug should still error with the specific Slugs should not contain slashes error.

You're right, I was confused. Your new solution sounds good.

It looks like this did something similar and suggests slightly different behaviour:
https://github.com/solid/community-server/blob/44d43456b168fc5e692e7ae1fcbe85d4beeb61b0/src/storage/DataAccessorBasedStore.ts#L199-L204

I chose to go for that behaviour, since it is consistent: resource slugs/path don't end in slash, directory slugs/path end in slash with the added requirement of the link header.

Since checking fot this behaviour is required in both setRepresentation() and createURI(), I've encapsulated it in a new method validateSlug(isContainer:boolean, suffix?: string): void.

I am effectively reusing the BadRequestHttpError mentioned above.

All unit tests now succeed, but now some integration tests seem to fail, I can't really figure out why though.

I am trying to fix most of the obvious failing ones (by adding a Link header, since there are some that just require that)

joachimvh · 2022-02-21T09:29:00Z

src/storage/DataAccessorBasedStore.ts

-    const isContainer = this.isNewContainer(representation.metadata, identifier.path);
+    const isContainer = this.isNewContainer(representation.metadata);
    // Solid, §3.1: "Paths ending with a slash denote a container resource."
    // https://solid.github.io/specification/protocol#uri-slash-semantics
-    if (isContainer !== isContainerIdentifier(identifier)) {
-      throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.');
-    }
+    this.validateSlug(isContainer, identifier.path);


This is the cause of your tests failing. When doing a PUT it is not required to have the correct link headers

When creating new resources, servers can determine an effective request URI’s type by examining the URI path ending

So adding checks here is going to cause all calls that depend on that behaviour to fail.

I got them all checked now. Changed the name of isNewContainer() to isContainerType to better accommodate what it does now. (only checking the metadata)

joachimvh

Some minor changes still required. Probably a consequence of the confusion with the previous setRepresentation behaviour 😅.

joachimvh · 2022-02-21T13:17:11Z

src/storage/DataAccessorBasedStore.ts

+    const potentialSlug = representation.metadata.get(SOLID_HTTP.slug)?.value;
+    if (potentialSlug && (isContainerPath(potentialSlug) !== isContainerIdentifier(identifier))) {


Suggested change

const potentialSlug = representation.metadata.get(SOLID_HTTP.slug)?.value;

if (potentialSlug && (isContainerPath(potentialSlug) !== isContainerIdentifier(identifier))) {

if (isContainerType(representation.metadata) !== isContainerIdentifier(identifier)) {

You're actually right that this is something that was checked here due to the isNewContainer call, but that was just a consequence of that check having been added to that function later. That's on me for saying we needed the exact same behaviour 😅. But the PUT calls don't care about the slug so this can be removed.

The type check I suggest above is not strictly necessary either as we set the types correctly afterwards, but is probably still a good idea to prevent users from making mistakes.

joachimvh · 2022-02-21T13:18:38Z

src/storage/DataAccessorBasedStore.ts

+   */
+  protected validateSlug(isContainer: boolean, suffix?: string): void {
+    if (suffix && (isContainer !== isContainerPath(suffix))) {
+      throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.');


Suggested change

throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.');

throw new BadRequestHttpError('Only slugs used to create containers can end with a `/`.');

joachimvh · 2022-02-21T13:20:28Z

src/storage/DataAccessorBasedStore.ts

+  /**
+   * Validates if the slug and headers are valid.
+   * Errors if slug exists, ends on slash, but ContainerType Link header is NOT present
+   * OR if slug exists, does not end on slash, but a ContainerType Link header IS present


Suggested change

* OR if slug exists, does not end on slash, but a ContainerType Link header IS present

This case should not fail. We are just being permissive and allowing users to end their slug with a slash if they create a container, but it is not a requirement.

joachimvh · 2022-02-21T13:21:29Z

test/integration/ServerFetch.test.ts

-      body: '<a:b> <a:b> <a:b>.',
+      body: `<a:b> <a:b> <a:b>.`,


I assume this is an artifact of previous changes? 😄

src/storage/DataAccessorBasedStore.ts

Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>

joachimvh

Looks good now, only thing is that I see that there are several tests where the line representation.metadata.add(RDF.type, LDP.terms.Container); was added. But this should not be necessary right? Can you remove those so the tests stay mostly the same?

Falx · 2022-02-23T12:07:36Z

Ah yes, I will check against the base and remove where not needed

joachimvh

Looks good, thanks.

fix: slugs ending on slash with link header cannot create containers

0f1128d

Falx force-pushed the feat/slug-with-slash branch from 44d4345 to 0f1128d Compare February 17, 2022 10:59

Falx requested a review from joachimvh February 17, 2022 11:08

joachimvh reviewed Feb 17, 2022

View reviewed changes

refactor(DataAccessorBasedStore): removing duplicate code and errors

c0e9c9e

Falx requested a review from joachimvh February 17, 2022 15:08

test(ServerFetch): fix integration tests that create a container

3880435

joachimvh reviewed Feb 21, 2022

View reviewed changes

Falx added 2 commits February 21, 2022 13:53

fix: Reinstate original checks in setResource to let tests succeed

472caea

test: change to container paths to end in / on POST only

07b8e3c

Falx requested a review from joachimvh February 21, 2022 13:14

joachimvh requested changes Feb 21, 2022

View reviewed changes

refactor: incorporate review changes

3ec0101

joachimvh reviewed Feb 22, 2022

View reviewed changes

src/storage/DataAccessorBasedStore.ts Outdated Show resolved Hide resolved

Falx and others added 2 commits February 23, 2022 10:13

fix: Error check was too strict

08584e8

Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>

test: make testcase to test allowed slash behaviour

7c98baa

Falx requested a review from joachimvh February 23, 2022 11:05

joachimvh requested changes Feb 23, 2022

View reviewed changes

Falx added 2 commits February 23, 2022 13:18

test: removed unnecessary code from tests

dffc1f8

test: remove metadata line and duplicate tests

d9497ea

joachimvh approved these changes Feb 23, 2022

View reviewed changes

joachimvh merged commit 5965268 into versions/4.0.0 Feb 23, 2022

joachimvh deleted the feat/slug-with-slash branch February 23, 2022 13:35

joachimvh mentioned this pull request Feb 23, 2022

Slugs should not create containers #1082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: slugs ending on slash with link header cannot create containers #1171

fix: slugs ending on slash with link header cannot create containers #1171

Falx commented Feb 17, 2022

Falx commented Feb 17, 2022

joachimvh Feb 17, 2022

Falx Feb 17, 2022

joachimvh Feb 17, 2022

Falx Feb 17, 2022

Falx Feb 18, 2022

joachimvh Feb 21, 2022

Falx Feb 21, 2022

joachimvh left a comment

joachimvh Feb 21, 2022

joachimvh Feb 21, 2022

joachimvh Feb 21, 2022

joachimvh Feb 21, 2022

joachimvh left a comment

Falx commented Feb 23, 2022

joachimvh left a comment

		const potentialSlug = representation.metadata.get(SOLID_HTTP.slug)?.value;
		if (potentialSlug && (isContainerPath(potentialSlug) !== isContainerIdentifier(identifier))) {

	const potentialSlug = representation.metadata.get(SOLID_HTTP.slug)?.value;
	if (potentialSlug && (isContainerPath(potentialSlug) !== isContainerIdentifier(identifier))) {
	if (isContainerType(representation.metadata) !== isContainerIdentifier(identifier)) {

	throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.');
	throw new BadRequestHttpError('Only slugs used to create containers can end with a `/`.');

fix: slugs ending on slash with link header cannot create containers #1171

fix: slugs ending on slash with link header cannot create containers #1171

Conversation

Falx commented Feb 17, 2022

📁 Related issues

✍️ Description

✅ PR check list

Falx commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimvh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimvh left a comment

Choose a reason for hiding this comment

Falx commented Feb 23, 2022

joachimvh left a comment

Choose a reason for hiding this comment