fix: %2F not handled correctly in file backend #1184 #1265

wkerckho · 2022-04-14T14:34:19Z

📁 Related issues

✍️ Description

Modified encodeUriPathComponents and decodeUriPathComponents in PathUtil.ts so that url encoded path separator characters (/, \) no longer erroneously impact the file path that is targeted when using a file backend. The implementation uses two new helper functions escapeCodecExceptionsForEncoding and escapeCodecExceptionsForDecoding that prepare a path part for encoding and decoding respectively, by replacing all instances of the matching encoded file separator characters with a character or character sequence that results in the follow-up encode/decode to have no effect. The code can be extended with additional path separator characters (or other exceptional characters) to ignore during encoding/decoding, by adding the respective characters to the codecExceptions constant initializer.

As requested, I've extended the integration tests with FileBackendEncodedSlashHandling.test.ts, which checks the operations that could not be executed successfully before this fix.

✅ 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.
any relevant documentation was updated to reflect the changes in this PR.

RubenVerborgh

Good work!
A couple of nitpicks (given how complicated this particular escaping is).

RubenVerborgh · 2022-04-14T15:15:45Z

src/util/PathUtil.ts

 import { BadRequestHttpError } from './errors/BadRequestHttpError';

+// Characters (mapped to their encoded representation) to ignore when URL decoding the URI path to a file path.
+const codecExceptions = new Map([ '/', '\\' ].map((item): [string, string] => [ item, encodeURIComponent(item) ]));


Let's name this pathComponentDelimiters (as per solid/specification#381 (comment)) and use a regular JavaScript object, which are the more straightforward option when keys are strings (or symbols). We can possibly use object.fromEntries.

RubenVerborgh · 2022-04-14T15:19:02Z

src/util/PathUtil.ts

+ * @param path - The path to apply the escaping on.
+ * @returns A copy of the input path that is safe to apply URL decoding on.
+ */
+function escapeCodecExceptionsForDecoding(path: string): string {


Suggested change

function escapeCodecExceptionsForDecoding(path: string): string {

function preventDelimiterDecoding(pathComponent: string): string {

RubenVerborgh · 2022-04-14T15:28:46Z

src/util/PathUtil.ts

+  codecExceptions.forEach((value, key): void => {
+    path = path.replace(codecExceptionsRegexCache.get(key)!, `%25${value.slice(1)}`);
+  });


A (double) loop is expensive here; I think we should just go for

Suggested change

codecExceptions.forEach((value, key): void => {

path = path.replace(codecExceptionsRegexCache.get(key)!, `%25${value.slice(1)}`);

});

return pathComponent.replace(/\/|\\/gu, encodeComponentDelimiter);

with

const encodedDelimiters = { '/': '%25%2f', '\\': '%25%5c' }; function encodeComponentDelimiter(delimiter: string) { return encodedDelimiters[delimiter]; }

RubenVerborgh · 2022-04-14T15:32:55Z

src/util/PathUtil.ts

+ * @param path - The path to apply the escaping on.
+ * @returns A copy of the input path that is safe to apply URL encoding on.
+ */
+function escapeCodecExceptionsForEncoding(path: string): string {


Suggested change

function escapeCodecExceptionsForEncoding(path: string): string {

function preventDelimiterEncoding(pathComponent: string): string {

joachimvh

Not much to add besides Ruben's comment except I would extend the integration tests more to really make sure there are no potential issues here.

joachimvh · 2022-04-15T08:00:12Z

src/util/PathUtil.ts

@@ -115,14 +149,16 @@ export function toCanonicalUriPath(path: string): string {
 * Decodes all components of a URI path.


Should have some more comments here (and in encodeUriPathComponents) to specify exactly that we use these functions to convert between URIs and file paths.

joachimvh · 2022-04-15T08:06:16Z

test/integration/config/file-backend-encoded-slash-handling.json

@@ -0,0 +1,54 @@
+{


This is the same config as server-memory.json but with a file backend right? Would just call it server-file.json then.

joachimvh · 2022-04-15T08:09:05Z

test/integration/FileBackendEncodedSlashHandling.test.ts

+    await app.stop();
+  });
+
+  it('can put a document for which the URI path contains url encoded separator characters.', async(): Promise<void> => {


Add a check here that the resource is not accessible through ${baseUrl}/c1/c2/t1/. Also check that the new resource is not a container by doing a GET and checking the headers.

joachimvh · 2022-04-15T08:09:44Z

test/integration/FileBackendEncodedSlashHandling.test.ts

+    expect(res.headers.get('location')).toBe(`${baseUrl}${slug}`);
+  });
+
+  it('prevents accessing a document via a different identifier that results in the same path after url decoding.',


Would also add the reverse check just to be sure.

joachimvh · 2022-04-15T08:11:14Z

test/integration/FileBackendEncodedSlashHandling.test.ts

+
+const rootFilePath = getTestFolder('file-backend-encoded-slash-handling');
+
+describe('A server with a file backend storage', (): void => {


For each test I would add an additional check that looks at the file name on disk since we know what it should be and where it should be located.

wkerckho · 2022-04-19T12:21:50Z

I've updated my changes based on your feedback! (Sorry for the delay)

RubenVerborgh · 2022-04-19T19:03:07Z

src/util/PathUtil.ts

+const pathComponentDelimiters = [ '/', '\\' ];
+// Regex to find all instances of encoded path component delimiters.
+const encodedDelimiterRegex = new RegExp(
+  `${pathComponentDelimiters.map((item): string => encodeURIComponent(item)).join('|')}`, 'giu',


I'd do

Suggested change

`${pathComponentDelimiters.map((item): string => encodeURIComponent(item)).join('|')}`, 'giu',

`${pathComponentDelimiters.map((delimiter): string => encodeURIComponent(item)).join('|')}`, 'giu',

throughout (especially with the arrays below at 17 and 22) but that's a crazy nit

wkerckho · 2022-04-20T12:30:57Z

Renamed item argument to delimiter in the sections Ruben mentioned.
Rebased from latest main (now that v4 has been merged).

RubenVerborgh · 2022-04-20T14:17:17Z

Super-clean now. Love it ❤️

Fix

bourgeoa · 2022-04-21T21:27:27Z

If I understand well, the above code only avoid %conversion of %2F

Trying the same concept on NSS. I stumble on the following issue : I run tests with the 2 different %encoded slash %2f and %2F and both where needed. They both %decode to /

The except encode/decode function should consider both cases.
I am not sure but it seems that you must also check for double %encode/decode because decoding %252F --> %2F and this is not %re-encoded.

RubenVerborgh · 2022-04-21T21:38:00Z

I run tests with the 2 different %encoded slash %2f and %2F and both where needed.

Clever—lemme fix that indeed!

bourgeoa · 2022-04-21T22:06:20Z

Here is my PR nodeSolidServer/node-solid-server#1690 can you have a look

RubenVerborgh · 2022-04-21T22:25:18Z

@bourgeoa That's quite cool; I learnt a clever trick there!

wkerckho added the semver.patch Does not require a minor or major version bump label Apr 14, 2022

wkerckho requested a review from joachimvh April 14, 2022 14:34

RubenVerborgh self-requested a review April 14, 2022 15:07

RubenVerborgh reviewed Apr 14, 2022

View reviewed changes

joachimvh reviewed Apr 15, 2022

View reviewed changes

wkerckho force-pushed the fix/issue-1184 branch from ea4fbb2 to c3daf12 Compare April 19, 2022 12:18

RubenVerborgh assigned joachimvh Apr 19, 2022

RubenVerborgh approved these changes Apr 19, 2022

View reviewed changes

wkerckho force-pushed the fix/issue-1184 branch from c3daf12 to d0c7f01 Compare April 20, 2022 12:29

wkerckho force-pushed the fix/issue-1184 branch from d0c7f01 to 08a7227 Compare April 21, 2022 12:05

joachimvh approved these changes Apr 21, 2022

View reviewed changes

fix: %2F not handled correctly in file backend #1184

18d9f2c

Fix

wkerckho force-pushed the fix/issue-1184 branch from 08a7227 to 18d9f2c Compare April 21, 2022 12:56

joachimvh merged commit dbdb9b4 into main Apr 21, 2022

joachimvh deleted the fix/issue-1184 branch April 21, 2022 13:17

	function escapeCodecExceptionsForDecoding(path: string): string {
	function preventDelimiterDecoding(pathComponent: string): string {

	function escapeCodecExceptionsForEncoding(path: string): string {
	function preventDelimiterEncoding(pathComponent: string): string {

		@@ -115,14 +149,16 @@ export function toCanonicalUriPath(path: string): string {
		* Decodes all components of a URI path.


		const rootFilePath = getTestFolder('file-backend-encoded-slash-handling');

		describe('A server with a file backend storage', (): void => {

	`${pathComponentDelimiters.map((item): string => encodeURIComponent(item)).join('\|')}`, 'giu',
	`${pathComponentDelimiters.map((delimiter): string => encodeURIComponent(item)).join('\|')}`, 'giu',

fix: %2F not handled correctly in file backend #1184 #1265

fix: %2F not handled correctly in file backend #1184 #1265

Uh oh!

Conversation

wkerckho commented Apr 14, 2022

📁 Related issues

✍️ Description

✅ PR check list

Uh oh!

RubenVerborgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joachimvh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkerckho commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkerckho commented Apr 20, 2022

Uh oh!

RubenVerborgh commented Apr 20, 2022

Uh oh!

bourgeoa commented Apr 21, 2022

Uh oh!

RubenVerborgh commented Apr 21, 2022

Uh oh!

bourgeoa commented Apr 21, 2022

Uh oh!

RubenVerborgh commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants