Skip to content

Commit

Permalink
fix: Fix searching for unescaped paths via regex
Browse files Browse the repository at this point in the history
This fixes #1037.

It turned out that when creating RegExp objects from strings,
TypeScript automatically escapes un-escaped forward slashes (/).

But the previous validation code in validateAndConstruct() was wrongly
truncating queries at the first unescaped forward slash.

By removing that validation, it is now possible to use unescaped paths
in regular expressions (providing that they don't have any other
unescaped special characters, of course).

And if the regular expression does have an error, the user will now get
a meaningful error message via the exception thrown by the RegExp
constructor.

Previous searches where '/' characters are escaped will still work,
but this escaping is no longer necessary.
  • Loading branch information
claremacrae committed Jul 16, 2023
1 parent 1302cac commit 1be22e3
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 28 deletions.
3 changes: 1 addition & 2 deletions src/Query/Matchers/RegexMatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ export class RegexMatcher extends IStringMatcher {
* @throws {SyntaxError} Throws an exception if there was an error in {@link regexInput}.
*/
public static validateAndConstruct(regexInput: string): RegexMatcher | null {
// Courtesy of https://stackoverflow.com/questions/17843691/javascript-regex-to-match-a-regex
const regexPattern = /\/((?![*+?])(?:[^\r\n[/\\]|\\.|\[(?:[^\r\n\]\\]|\\.)*])+)\/([igmu]*)/;
const regexPattern = /^\/(.+)\/([igmu]*)/;
const query = regexInput.match(regexPattern);

if (query !== null) {
Expand Down
27 changes: 6 additions & 21 deletions tests/Query/Filter/PathField.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,33 +76,18 @@ describe('path', () => {
});
});

describe('invalid unescaped slash should give helpful error text and not search', () => {
describe('should use whole path with un-escaped slashes in query', () => {
const filterWithUnescapedSlashes = new PathField().createFilterOrErrorMessage(
String.raw`path regex matches /a/b/c/d/`,
);

// This test demonstrates the issue logged in
// https://github.com/obsidian-tasks-group/obsidian-tasks/issues/1037
// 'Work out how to prevent `path regex matches /a/b/c/d/` from
// confusingly only searching `path regex matches /a/`.
// All these tests are marked as 'failing' because the code currently accepts
// an invalid search.
it.failing('should not be valid', () => {
expect(filterWithUnescapedSlashes).not.toBeValid();
it('should escape backslashes in query automatically', () => {
expect(filterWithUnescapedSlashes).toBeValid();
expect(filterWithUnescapedSlashes).toHaveExplanation("using regex: 'a\\/b\\/c\\/d' with no flags");
});

it.failing('should have a meaningful error message', () => {
// The error message does not have to be exactly this.
// The main thing is that it should convey what the user needs to do
// to fix the expression.
expect(filterWithUnescapedSlashes.error).toEqual(
'An unescaped delimiter must be escaped; in most languages with a backslash (\\)',
);
});

it.failing('should not match a subset of requested path', () => {
// Once the issue is fixed, and filterWithUnescapedSlashes is not valid,
// this test should be deleted.
it('should match the requested path', () => {
expect(filterWithUnescapedSlashes).toMatchTaskWithPath('/a/b/c/d/e.md');
expect(filterWithUnescapedSlashes).not.toMatchTaskWithPath('/a/b.md');
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ description regex matches / / =>
using regex: ' ' with no flags

description regex matches /#context/pc_photos|#context/pc_clare|#context/pc_macbook/i =>
using regex: '#context' with no flags
using regex: '#context\/pc_photos|#context\/pc_clare|#context\/pc_macbook' with flag 'i'

description regex matches /#context\/pc_photos|#context\/pc_clare|#context\/pc_macbook/i =>
using regex: '#context\/pc_photos|#context\/pc_clare|#context\/pc_macbook' with flag 'i'
Expand Down Expand Up @@ -41,7 +41,7 @@ filename regex does not match /^Tasks User Support Kanban\.md$/ =>
using regex: '^Tasks User Support Kanban\.md$' with no flags

folder regex matches /root/sub-folder/sub-sub-folder/ =>
using regex: 'root' with no flags
using regex: 'root\/sub-folder\/sub-sub-folder' with no flags

folder regex matches /root\/sub-folder\/sub-sub-folder/ =>
using regex: 'root\/sub-folder\/sub-sub-folder' with no flags
Expand Down Expand Up @@ -77,7 +77,7 @@ path regex matches /clare/i =>
using regex: 'clare' with flag 'i'

path regex matches /root/sub-folder/sub-sub-folder/index\.md/ =>
using regex: 'root' with no flags
using regex: 'root\/sub-folder\/sub-sub-folder\/index\.md' with no flags

path regex matches /root\/sub-folder\/sub-sub-folder\/index\.md/ =>
using regex: 'root\/sub-folder\/sub-sub-folder\/index\.md' with no flags
Expand Down
4 changes: 2 additions & 2 deletions tests/Query/Matchers/RegexMatcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ describe('RegexMatcher', () => {
});

describe('RegexMatcher source', () => {
it.failing('should allow multiple slashes in source', () => {
it('should allow multiple slashes in source', () => {
const matcher = RegexMatcher.validateAndConstruct('/a/b/c/d/');
expect(matcher!.regex.source).toEqual(String.raw`a\/b\/c\/d`);
});

it.failing('should allow multiple slashes and delimiters in source', () => {
it('should allow multiple slashes and delimiters in source', () => {
const matcher = RegexMatcher.validateAndConstruct('//a/b/c/d//');
expect(matcher!.regex.source).toEqual(String.raw`\/a\/b\/c\/d\/`);
});
Expand Down

0 comments on commit 1be22e3

Please sign in to comment.