fix: Prevent slugs with trailing slashes for non-container resources

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

* refactor(DataAccessorBasedStore): removing duplicate code and errors

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

* fix: Reinstate original checks in setResource to let tests succeed

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

* refactor: incorporate review changes

* fix: Error check was too strict

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

* test: make testcase to test allowed slash behaviour

* test: removed unnecessary code from tests

* test: remove metadata line and duplicate tests

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

src/storage/DataAccessorBasedStore.ts

@@ -196,10 +196,10 @@ export class DataAccessorBasedStore implements ResourceStore {
       throw new ConflictHttpError(`${identifier.path} conflicts with existing path ${oldMetadata.identifier.value}`);
     }
 
-    const isContainer = this.isNewContainer(representation.metadata, identifier.path);
     // 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)) {
+    const isContainer = isContainerIdentifier(identifier);
+    if (!isContainer && this.isContainerType(representation.metadata)) {
       throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.');
     }
 
@@ -473,12 +473,25 @@ export class DataAccessorBasedStore implements ResourceStore {
    * @param slug - Slug to use for the new URI.
    */
   protected createURI(container: ResourceIdentifier, isContainer: boolean, slug?: string): ResourceIdentifier {
+    this.validateSlug(isContainer, slug);
     const base = ensureTrailingSlash(container.path);
     const name = (slug && this.cleanSlug(slug)) ?? uuid();
     const suffix = isContainer ? '/' : '';
     return { path: `${base}${name}${suffix}` };
   }
 
+  /**
+   * Validates if the slug and headers are valid.
+   * Errors if slug exists, ends on slash, but ContainerType Link header is NOT present
+   * @param isContainer - Is the slug supposed to represent a container?
+   * @param slug - Is the requested slug (if any).
+   */
+  protected validateSlug(isContainer: boolean, slug?: string): void {
+    if (slug && isContainerPath(slug) && !isContainer) {
+      throw new BadRequestHttpError('Only slugs used to create containers can end with a `/`.');
+    }
+  }
+
   /**
    * Clean http Slug to be compatible with the server. Makes sure there are no unwanted characters
    * e.g.: cleanslug('&%26') returns '%26%26'
@@ -501,7 +514,7 @@ export class DataAccessorBasedStore implements ResourceStore {
   protected async createSafeUri(container: ResourceIdentifier, metadata: RepresentationMetadata):
   Promise<ResourceIdentifier> {
     // Get all values needed for naming the resource
-    const isContainer = this.isNewContainer(metadata);
+    const isContainer = this.isContainerType(metadata);
     const slug = metadata.get(SOLID_HTTP.slug)?.value;
     metadata.removeAll(SOLID_HTTP.slug);
 
@@ -526,16 +539,11 @@ export class DataAccessorBasedStore implements ResourceStore {
 
   /**
    * Checks if the given metadata represents a (potential) container,
-   * both based on the metadata and the URI.
+   * based on the metadata.
    * @param metadata - Metadata of the (new) resource.
-   * @param suffix - Suffix of the URI. Can be the full URI, but only the last part is required.
    */
-  protected isNewContainer(metadata: RepresentationMetadata, suffix?: string): boolean {
-    if (this.hasContainerType(metadata.getAll(RDF.type))) {
-      return true;
-    }
-    const slug = suffix ?? metadata.get(SOLID_HTTP.slug)?.value;
-    return Boolean(slug && isContainerPath(slug));
+  protected isContainerType(metadata: RepresentationMetadata): boolean {
+    return this.hasContainerType(metadata.getAll(RDF.type));
   }
 
   /**

test/integration/LdpHandlerWithoutAuth.test.ts

@@ -265,14 +265,14 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
   });
 
   it('can create a container with a diamond identifier in the data.', async(): Promise<void> => {
-    const slug = 'my-container';
+    const slug = 'my-container/';
 
     const body = '<> <http://www.w3.org/2000/01/rdf-schema#label> "My Container" .';
     let response = await postResource(baseUrl, { isContainer: true, contentType: 'text/turtle', slug, body });
-    expect(response.headers.get('location')).toBe(`${baseUrl}${slug}/`);
+    expect(response.headers.get('location')).toBe(`${baseUrl}${slug}`);
 
     // GET
-    const containerUrl = `${baseUrl}${slug}/`;
+    const containerUrl = `${baseUrl}${slug}`;
     response = await getResource(containerUrl);
 
     await expectQuads(response, [

test/integration/ServerFetch.test.ts

@@ -1,5 +1,6 @@
 import fetch from 'cross-fetch';
 import type { App } from '../../src/init/App';
+import { LDP } from '../../src/util/Vocabularies';
 import { getPort } from '../util/Util';
 import { getDefaultVariables, getTestConfigPath, instantiateFromConfig } from './Config';
 
@@ -112,6 +113,7 @@ describe('A Solid server', (): void => {
       headers: {
         'content-type': 'text/turtle',
         slug: 'containerPOST/',
+        link: `<${LDP.Container}>; rel="type"`,
       },
       body: '<a:b> <a:b> <a:b>.',
     });

test/unit/storage/DataAccessorBasedStore.test.ts

@@ -299,6 +299,31 @@ describe('A DataAccessorBasedStore', (): void => {
       });
     });
 
+    it('errors on a slug ending on / without Link rel:type Container header.', async(): Promise<void> => {
+      const resourceID = { path: root };
+      representation.metadata.removeAll(RDF.type);
+      representation.metadata.add(SOLID_HTTP.slug, 'noContainer/');
+      representation.data = guardedStreamFrom([ `` ]);
+      const result = store.addResource(resourceID, representation);
+
+      await expect(result).rejects.toThrow(BadRequestHttpError);
+      await expect(result).rejects
+        .toThrow('Only slugs used to create containers can end with a `/`.');
+    });
+
+    it('creates a URI when the incoming slug does not end with /, ' +
+      'but has a Link rel:type Container header.', async(): Promise<void> => {
+      const resourceID = { path: root };
+      representation.metadata.removeAll(RDF.type);
+      representation.metadata.add(RDF.type, LDP.terms.Container);
+      representation.metadata.add(SOLID_HTTP.slug, 'newContainer');
+      representation.data = guardedStreamFrom([ `` ]);
+      const result = await store.addResource(resourceID, representation);
+      expect(result).toEqual({
+        path: `${root}newContainer/`,
+      });
+    });
+
     it('generates a new URI if adding the slug would create an existing URI.', async(): Promise<void> => {
       const resourceID = { path: root };
       representation.metadata.add(SOLID_HTTP.slug, 'newName');
@@ -389,7 +414,7 @@ describe('A DataAccessorBasedStore', (): void => {
       mock.mockRestore();
     });
 
-    it('will error if the ending slash does not match its resource type.', async(): Promise<void> => {
+    it('will error if path does not end in slash and does not match its resource type.', async(): Promise<void> => {
       const resourceID = { path: `${root}resource` };
       representation.metadata.add(RDF.type, LDP.terms.Container);
       await expect(store.setRepresentation(resourceID, representation)).rejects.toThrow(