From 28c0eb7e887f907fc4ca3a5045d9eb71cf0b0491 Mon Sep 17 00:00:00 2001 From: Arthur Joppart <38424924+BelgianNoise@users.noreply.github.com> Date: Wed, 24 Feb 2021 12:03:41 +0100 Subject: [PATCH] Correctly handle slugs in POST requests * bug: correctly handle slug in POST request * bug: disallow slashes in slug + modified tests * fix: fixed tests to work with PUT instead of POST+slug * fix: fixed tests failing in ci * fix: adapted to reviews * fix: adapted to review --- src/storage/DataAccessorBasedStore.ts | 19 ++- test/integration/LdpHandlerWithAuth.test.ts | 39 +++-- .../integration/LdpHandlerWithoutAuth.test.ts | 154 ++++++++++-------- test/integration/SparqlStorage.test.ts | 5 +- .../storage/DataAccessorBasedStore.test.ts | 19 +++ test/util/TestHelpers.ts | 21 +-- 6 files changed, 154 insertions(+), 103 deletions(-) diff --git a/src/storage/DataAccessorBasedStore.ts b/src/storage/DataAccessorBasedStore.ts index c28a21e82d..52a08d450a 100644 --- a/src/storage/DataAccessorBasedStore.ts +++ b/src/storage/DataAccessorBasedStore.ts @@ -22,6 +22,7 @@ import { isContainerIdentifier, isContainerPath, trimTrailingSlashes, + toCanonicalUriPath, } from '../util/PathUtil'; import { parseQuads } from '../util/QuadUtil'; import { generateResourceQuads } from '../util/ResourceUtil'; @@ -347,8 +348,22 @@ export class DataAccessorBasedStore implements ResourceStore { * @param slug - Slug to use for the new URI. */ protected createURI(container: ResourceIdentifier, isContainer: boolean, slug?: string): ResourceIdentifier { - return { path: - `${ensureTrailingSlash(container.path)}${slug ? trimTrailingSlashes(slug) : uuid()}${isContainer ? '/' : ''}` }; + const base = ensureTrailingSlash(container.path); + const name = (slug && this.cleanSlug(slug)) ?? uuid(); + const suffix = isContainer ? '/' : ''; + return { path: `${base}${name}${suffix}` }; + } + + /** + * Clean http Slug to be compatible with the server. Makes sure there are no unwanted characters + * e.g.: cleanslug('&%26') returns '%26%26' + * @param slug - the slug to clean + */ + protected cleanSlug(slug: string): string { + if (/\/[^/]/u.test(slug)) { + throw new BadRequestHttpError('Slugs should not contain slashes'); + } + return toCanonicalUriPath(trimTrailingSlashes(slug)); } /** diff --git a/test/integration/LdpHandlerWithAuth.test.ts b/test/integration/LdpHandlerWithAuth.test.ts index 4034a3da2b..7c192084dd 100644 --- a/test/integration/LdpHandlerWithAuth.test.ts +++ b/test/integration/LdpHandlerWithAuth.test.ts @@ -64,22 +64,23 @@ describe.each(stores)('An LDP handler with auth using %s', (name, { storeUrn, te await aclHelper.setSimpleAcl({ read: true, write: true, append: true, control: false }, 'agent'); // Create file + const filePath = 'testfile2.txt'; + const fileUrl = `${BASE}/${filePath}`; let response = await resourceHelper.createResource( - '../assets/testfile2.txt', 'testfile2.txt', 'text/plain', + '../assets/testfile2.txt', filePath, 'text/plain', ); - const id = response._getHeaders().location; // Get file - response = await resourceHelper.getResource(id); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response._getBuffer().toString()).toContain('TESTFILE2'); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${fileUrl}.acl>; rel="acl"`); expect(response.getHeaders()['wac-allow']).toBe('user="read write append",public="read write append"'); // DELETE file - await resourceHelper.deleteResource(id); - await resourceHelper.shouldNotExist(id); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); }); it('can not add a file to the store if not allowed.', async(): Promise => { @@ -87,8 +88,9 @@ describe.each(stores)('An LDP handler with auth using %s', (name, { storeUrn, te await aclHelper.setSimpleAcl({ read: true, write: true, append: true, control: false }, 'authenticated'); // Try to create file + const filePath = 'testfile2.txt'; const response = await resourceHelper.createResource( - '../assets/testfile2.txt', 'testfile2.txt', 'text/plain', true, + '../assets/testfile2.txt', filePath, 'text/plain', true, ); expect(response.statusCode).toBe(401); }); @@ -98,8 +100,9 @@ describe.each(stores)('An LDP handler with auth using %s', (name, { storeUrn, te await aclHelper.setSimpleAcl({ read: true, write: false, append: false, control: false }, 'agent'); // Try to create file + const filePath = 'testfile2.txt'; let response = await resourceHelper.createResource( - '../assets/testfile2.txt', 'testfile2.txt', 'text/plain', true, + '../assets/testfile2.txt', filePath, 'text/plain', true, ); expect(response.statusCode).toBe(401); @@ -120,17 +123,21 @@ describe.each(stores)('An LDP handler with auth using %s', (name, { storeUrn, te await aclHelper.setSimpleAcl({ read: true, write: false, append: true, control: false }, 'agent'); // Add a file - let response = await resourceHelper.createResource( - '../assets/testfile2.txt', 'testfile2.txt', 'text/plain', true, + const filePath = 'testfile2.txt'; + let response = await resourceHelper.performRequestWithBody( + new URL(`${BASE}/`), + 'POST', + { + 'content-type': 'text/plain', + 'transfer-encoding': 'chunked', + slug: filePath, + }, + Buffer.from('data'), ); expect(response.statusCode).toBe(201); - const id = response._getHeaders().location; - response = await resourceHelper.performRequestWithBody( - new URL(id), - 'PUT', - { 'content-type': 'text/plain', 'transfer-encoding': 'chunked' }, - Buffer.from('data'), + response = await resourceHelper.createResource( + '../assets/testfile2.txt', filePath, 'text/plain', true, ); expect(response.statusCode).toBe(401); }); diff --git a/test/integration/LdpHandlerWithoutAuth.test.ts b/test/integration/LdpHandlerWithoutAuth.test.ts index 5f52a0e6ee..be960b7220 100644 --- a/test/integration/LdpHandlerWithoutAuth.test.ts +++ b/test/integration/LdpHandlerWithoutAuth.test.ts @@ -90,187 +90,203 @@ describe.each(stores)('An LDP handler without auth using %s', (name, { storeUrn, it('can add a file to the store, read it and delete it.', async(): Promise => { - // POST + const filePath = 'testfile0.txt'; + const fileUrl = `${BASE}/${filePath}`; + // PUT let response = await resourceHelper.createResource( - '../assets/testfile0.txt', 'testfile0.txt', 'text/plain', + '../assets/testfile0.txt', filePath, 'text/plain', ); - const id = response._getHeaders().location; // GET - response = await resourceHelper.getResource(id); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response._getBuffer().toString()).toContain('TESTFILE0'); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${fileUrl}.acl>; rel="acl"`); expect(response.getHeaders()['accept-patch']).toBe('application/sparql-update'); expect(response.getHeaders()['ms-author-via']).toBe('SPARQL'); // DELETE - await resourceHelper.deleteResource(id); - await resourceHelper.shouldNotExist(id); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); }); it('can add and overwrite a file.', async(): Promise => { + const filePath = 'file.txt'; + const fileUrl = `${BASE}/${filePath}`; + // PUT let response = await resourceHelper.createResource( - '../assets/testfile0.txt', 'file.txt', 'text/plain', + '../assets/testfile0.txt', filePath, 'text/plain', ); - const id = response._getHeaders().location; // GET - response = await resourceHelper.getResource(id); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response._getBuffer().toString()).toContain('TESTFILE0'); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${fileUrl}.acl>; rel="acl"`); // PUT response = await resourceHelper.replaceResource( - '../assets/testfile1.txt', id, 'text/plain', + '../assets/testfile1.txt', fileUrl, 'text/plain', ); // GET - response = await resourceHelper.getResource(id); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response._getBuffer().toString()).toContain('TESTFILE1'); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${fileUrl}.acl>; rel="acl"`); // DELETE - await resourceHelper.deleteResource(id); - await resourceHelper.shouldNotExist(id); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); }); it('can create a folder and delete it.', async(): Promise => { - // POST - let response = await resourceHelper.createContainer('secondfolder/'); - const id = response._getHeaders().location; + const containerPath = 'secondfolder/'; + const containerUrl = `${BASE}/${containerPath}`; + // PUT + let response = await resourceHelper.createContainer(containerPath); // GET - response = await resourceHelper.getContainer(id); + response = await resourceHelper.getContainer(containerUrl); expect(response.statusCode).toBe(200); expect(response.getHeaders().link).toContain(`<${LDP.Container}>; rel="type"`); expect(response.getHeaders().link).toContain(`<${LDP.BasicContainer}>; rel="type"`); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${containerUrl}.acl>; rel="acl"`); // DELETE - await resourceHelper.deleteResource(id); - await resourceHelper.shouldNotExist(id); + await resourceHelper.deleteResource(containerUrl); + await resourceHelper.shouldNotExist(containerUrl); }); it('can make a folder and put a file in it.', async(): Promise => { // Create folder - await resourceHelper.createContainer('testfolder0/'); + const containerPath = 'testfolder0/'; + const containerUrl = `${BASE}/${containerPath}`; + await resourceHelper.createContainer(containerPath); // Create file + const filePath = 'testfolder0/testfile0.txt'; + const fileUrl = `${BASE}/${filePath}`; let response = await resourceHelper.createResource( - '../assets/testfile0.txt', 'testfolder0/testfile0.txt', 'text/plain', + '../assets/testfile0.txt', filePath, 'text/plain', ); - const id = response._getHeaders().location; // GET File - response = await resourceHelper.getResource(id); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${id}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${fileUrl}.acl>; rel="acl"`); // DELETE - await resourceHelper.deleteResource(id); - await resourceHelper.shouldNotExist(id); - await resourceHelper.deleteResource('http://test.com/testfolder0/'); - await resourceHelper.shouldNotExist('http://test.com/testfolder0/'); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); + await resourceHelper.deleteResource(containerUrl); + await resourceHelper.shouldNotExist(containerUrl); }); it('cannot remove a folder when the folder contains a file.', async(): Promise => { // Create folder - let response = await resourceHelper.createContainer('testfolder1/'); - const folderId = response._getHeaders().location; + const containerPath = 'testfolder1/'; + const containerUrl = `${BASE}/${containerPath}`; + let response = await resourceHelper.createContainer(containerPath); // Create file + const filePath = 'testfolder1/testfile0.txt'; + const fileUrl = `${BASE}/${filePath}`; await resourceHelper.createResource( - '../assets/testfile0.txt', 'testfolder1/testfile0.txt', 'text/plain', + '../assets/testfile0.txt', filePath, 'text/plain', ); // Try DELETE folder - response = await resourceHelper.performRequest(new URL(folderId), 'DELETE', {}); + response = await resourceHelper.performRequest(new URL(containerUrl), 'DELETE', {}); expect(response.statusCode).toBe(409); expect(response._getData()).toContain('ConflictHttpError: Can only delete empty containers.'); // DELETE - await resourceHelper.deleteResource('http://test.com/testfolder1/testfile0.txt'); - await resourceHelper.shouldNotExist('http://test.com/testfolder1/testfile0.txt'); - await resourceHelper.deleteResource(folderId); - await resourceHelper.shouldNotExist(folderId); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); + await resourceHelper.deleteResource(containerUrl); + await resourceHelper.shouldNotExist(containerUrl); }); it('cannot remove a folder when the folder contains a subfolder.', async(): Promise => { // Create folder - let response = await resourceHelper.createContainer('testfolder2/'); - const folderId = response._getHeaders().location; + const containerPath = 'testfolder2/'; + const containerUrl = `${BASE}/${containerPath}`; + let response = await resourceHelper.createContainer(containerPath); // Create subfolder - response = await resourceHelper.createContainer('testfolder2/subfolder0'); - const subFolderId = response._getHeaders().location; + const subContainerPath = `${containerPath}subfolder0/`; + const subContainerUrl = `${BASE}/${subContainerPath}`; + response = await resourceHelper.createContainer(subContainerPath); // Try DELETE folder - response = await resourceHelper.performRequest(new URL(folderId), 'DELETE', {}); + response = await resourceHelper.performRequest(new URL(containerUrl), 'DELETE', {}); expect(response.statusCode).toBe(409); expect(response._getData()).toContain('ConflictHttpError: Can only delete empty containers.'); // DELETE - await resourceHelper.deleteResource(subFolderId); - await resourceHelper.shouldNotExist(subFolderId); - await resourceHelper.deleteResource(folderId); - await resourceHelper.shouldNotExist(folderId); + await resourceHelper.deleteResource(subContainerUrl); + await resourceHelper.shouldNotExist(subContainerUrl); + await resourceHelper.deleteResource(containerUrl); + await resourceHelper.shouldNotExist(containerUrl); }); it('can read the contents of a folder.', async(): Promise => { // Create folder - let response = await resourceHelper.createContainer('testfolder3/'); - const folderId = response._getHeaders().location; + const containerPath = 'testfolder3/'; + const containerUrl = `${BASE}/${containerPath}`; + let response = await resourceHelper.createContainer(containerPath); // Create subfolder + const subContainerPath = `${containerPath}subfolder0/`; + const subContainerUrl = `${BASE}/${subContainerPath}`; response = await resourceHelper.createContainer('testfolder3/subfolder0/'); - const subFolderId = response._getHeaders().location; // Create file + const filePath = `${containerPath}testfile0.txt`; + const fileUrl = `${BASE}/${filePath}`; response = await resourceHelper.createResource( - '../assets/testfile0.txt', 'testfolder3/testfile0.txt', 'text/plain', + '../assets/testfile0.txt', filePath, 'text/plain', ); - const fileId = response._getHeaders().location; - response = await resourceHelper.getContainer(folderId); + response = await resourceHelper.getContainer(containerUrl); expect(response.statusCode).toBe(200); - expect(response._getData()).toContain(' .'); - expect(response._getData()).toContain(' .'); + expect(response._getData()).toContain(` <${subContainerUrl}> .`); + expect(response._getData()).toContain(` <${fileUrl}> .`); expect(response.getHeaders().link).toContain(`<${LDP.Container}>; rel="type"`); expect(response.getHeaders().link).toContain(`<${LDP.BasicContainer}>; rel="type"`); expect(response.getHeaders().link).toContain(`<${LDP.Resource}>; rel="type"`); - expect(response.getHeaders().link).toContain(`<${folderId}.acl>; rel="acl"`); + expect(response.getHeaders().link).toContain(`<${containerUrl}.acl>; rel="acl"`); // DELETE - await resourceHelper.deleteResource(fileId); - await resourceHelper.shouldNotExist(fileId); - await resourceHelper.deleteResource(subFolderId); - await resourceHelper.shouldNotExist(subFolderId); - await resourceHelper.deleteResource(folderId); - await resourceHelper.shouldNotExist(folderId); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); + await resourceHelper.deleteResource(subContainerUrl); + await resourceHelper.shouldNotExist(subContainerUrl); + await resourceHelper.deleteResource(containerUrl); + await resourceHelper.shouldNotExist(containerUrl); }); it('can upload and delete a image.', async(): Promise => { + const filePath = 'image.png'; + const fileUrl = `${BASE}/${filePath}`; let response = await resourceHelper.createResource( - '../assets/testimage.png', 'image.png', 'image/png', + '../assets/testimage.png', filePath, 'image/png', ); - const fileId = response._getHeaders().location; // GET - response = await resourceHelper.getResource(fileId); + response = await resourceHelper.getResource(fileUrl); expect(response.statusCode).toBe(200); expect(response._getHeaders()['content-type']).toBe('image/png'); // DELETE - await resourceHelper.deleteResource(fileId); - await resourceHelper.shouldNotExist(fileId); + await resourceHelper.deleteResource(fileUrl); + await resourceHelper.shouldNotExist(fileUrl); }); it('can create a container with a diamond identifier in the data.', async(): Promise => { diff --git a/test/integration/SparqlStorage.test.ts b/test/integration/SparqlStorage.test.ts index 995bbc6590..01ee11614c 100644 --- a/test/integration/SparqlStorage.test.ts +++ b/test/integration/SparqlStorage.test.ts @@ -35,9 +35,8 @@ describeIf('docker', 'A server with a SPARQL endpoint as storage', (): void => { it('can add a Turtle file to the store.', async(): Promise => { - // POST + // PUT const response = await resourceHelper.createResource('../assets/person.ttl', 'person', 'text/turtle'); - const id = response._getHeaders().location; - expect(id).toBeTruthy(); + expect(response).toBeTruthy(); }); }); diff --git a/test/unit/storage/DataAccessorBasedStore.test.ts b/test/unit/storage/DataAccessorBasedStore.test.ts index b0b40f2af4..e9268df121 100644 --- a/test/unit/storage/DataAccessorBasedStore.test.ts +++ b/test/unit/storage/DataAccessorBasedStore.test.ts @@ -291,6 +291,24 @@ describe('A DataAccessorBasedStore', (): void => { }); }); + it('generates http://test.com/%26%26 when slug is &%26.', async(): Promise => { + const resourceID = { path: root }; + representation.metadata.removeAll(RDF.type); + representation.metadata.add(HTTP.slug, '&%26'); + const result = await store.addResource(resourceID, representation); + expect(result).toEqual({ path: `${root}%26%26` }); + }); + + it('errors if the slug contains a slash.', async(): Promise => { + const resourceID = { path: root }; + representation.metadata.removeAll(RDF.type); + representation.data = guardedStreamFrom([ `` ]); + representation.metadata.add(HTTP.slug, 'sla/sh/es'); + const result = store.addResource(resourceID, representation); + await expect(result).rejects.toThrow(BadRequestHttpError); + await expect(result).rejects.toThrow('Slugs should not contain slashes'); + }); + it('errors if the slug would cause an auxiliary resource URI to be generated.', async(): Promise => { const resourceID = { path: root }; representation.metadata.removeAll(RDF.type); @@ -559,3 +577,4 @@ describe('A DataAccessorBasedStore', (): void => { }); }); }); + diff --git a/test/util/TestHelpers.ts b/test/util/TestHelpers.ts index 861efe7735..4f2fd7417f 100644 --- a/test/util/TestHelpers.ts +++ b/test/util/TestHelpers.ts @@ -2,7 +2,6 @@ import { EventEmitter } from 'events'; import { promises as fs } from 'fs'; import type { IncomingHttpHeaders } from 'http'; import { Readable } from 'stream'; -import * as url from 'url'; import type { MockResponse } from 'node-mocks-http'; import { createResponse } from 'node-mocks-http'; import type { ResourceStore, PermissionSet, HttpHandler, HttpRequest } from '../../src/'; @@ -93,24 +92,22 @@ export class ResourceHelper { return response; } - public async createResource(fileLocation: string, slug: string, contentType: string, mayFail = false): + public async createResource(fileLocation: string, path: string, contentType: string, mayFail = false): Promise> { const fileData = await fs.readFile( joinFilePath(__dirname, fileLocation), ); const response: MockResponse = await this.performRequestWithBody( - this.baseUrl, - 'POST', + new URL(path, this.baseUrl), + 'PUT', { 'content-type': contentType, - slug, 'transfer-encoding': 'chunked' }, fileData, ); if (!mayFail) { - expect(response.statusCode).toBe(201); + expect(response.statusCode).toBe(205); expect(response._getData()).toHaveLength(0); - expect(response._getHeaders().location).toContain(url.format(this.baseUrl)); } return response; } @@ -153,20 +150,18 @@ export class ResourceHelper { return response; } - public async createContainer(slug: string): Promise> { + public async createContainer(path: string): Promise> { const response: MockResponse = await this.performRequest( - this.baseUrl, - 'POST', + new URL(path, this.baseUrl), + 'PUT', { - slug, link: '; rel="type"', 'content-type': 'text/turtle', 'transfer-encoding': 'chunked', }, ); - expect(response.statusCode).toBe(201); + expect(response.statusCode).toBe(205); expect(response._getData()).toHaveLength(0); - expect(response._getHeaders().location).toContain(url.format(this.baseUrl)); return response; }