Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(common): make Location.normalize() return the correct path when the base path contains characters that interfere with regex syntax. #49181

Closed
wants to merge 2 commits into from
Closed

fix(common): make Location.normalize() return the correct path when the base path contains characters that interfere with regex syntax. #49181

wants to merge 2 commits into from

Conversation

wartab
Copy link
Contributor

@wartab wartab commented Feb 23, 2023

Fix the function stripping the base path from the URL, as the current implementation uses the base path as part of a regex, which wrongly makes paths fails that contain characters such as a parenthesis (example: C:/Users/MyUser(Test)/project).

Fixes #49179

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • [ x ] Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

When an Angular project build is served from a base path containing characters that are special characters in a regex, the Angular router does not work anymore. (for example C:/Users/MyUser(Test)/my-project).
Issue Number: 49179

What is the new behavior?

Paths containing such characters will not break routing anymore.

Does this PR introduce a breaking change?

  • Yes
  • [ x ] No

Other information

@wartab
fix(common): make Location.normalize() return the correct path when t…
be1ff4d 
…he base path contains characters that interfere with regex syntax.

Fix the function stripping the base path from the URL, as the current implementation uses the base path as part of a regex, which wrongly makes paths fails that contain characters such as a parenthesis (example: C:/Users/MyUser(Test)/project).

Fixes #49179
@atscott

This comment was marked as outdated.

Sign in to view
@atscott
Copy link
Contributor

atscott commented Feb 23, 2023

Actually, this fix seems reasonable. It's pretty close to what the revert would be anyways, but with some additional logic to fix the old bug again.

atscott
atscott requested changes Feb 23, 2023
View reviewed changes
packages/common/src/location/location.ts Outdated Show resolved Hide resolved
@wartab
Copy link
Contributor Author

wartab commented Feb 23, 2023 •
edited

Yes, my fix had your changes in mind, it was just to address the problem our customers were facing, because they somehow thought it was a good idea to add parenthesis to their username in Windows.

I don't have a preference for the solution, I generally tend to avoid regular expressions and operations that could be avoided when they are not needed, which is why I suggested my fix. But from my understanding this is executed only once (or very infrequently), so whatever solution suits me.

@atscott
fixup! fix(common): make Location.normalize() return the correct path…
61eb153 
… when the base path contains characters that interfere with regex syntax.
@atscott atscott added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker action: global presubmit The PR is in need of a google3 global presubmit and removed action: merge The PR is ready for merge by the caretaker labels Feb 24, 2023
@jessicajaniuk jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Feb 24, 2023
@ngbot ngbot bot added this to the Backlog milestone Feb 24, 2023
@atscott
Copy link
Contributor

atscott commented Feb 27, 2023

Global internal tests came back green as well

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Feb 27, 2023
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 3c9d49a.

AndrewKushnir pushed a commit that referenced this pull request Feb 27, 2023
@wartab @AndrewKushnir
fix(common): make Location.normalize() return the correct path when t…
f0e9260 
…he base path contains characters that interfere with regex syntax. (#49181)

Fix the function stripping the base path from the URL, as the current implementation uses the base path as part of a regex, which wrongly makes paths fails that contain characters such as a parenthesis (example: C:/Users/MyUser(Test)/project).

Fixes #49179

PR Close #49181
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 29, 2023
Update angular monorepo (#1801)
6e52606 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.2.0/15.2.4) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.2.0/15.2.4) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.2.0/15.2.4) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.2.0/15.2.4) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.2.0/15.2.4) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.2.0/15.2.4) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.2.0/15.2.4) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`15.2.0` -> `15.2.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.2.0/15.2.4) |
| [zone.js](https://github.com/angular/angular) ([changelog](https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md)) | dependencies | minor | [`0.12.0` -> `0.13.0`](https://renovatebot.com/diffs/npm/zone.js/0.12.0/0.13.0) |

---

### Release Notes

<details>
<summary>angular/angular (@&#8203;angular/animations)</summary>

### [`v15.2.4`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1524-2023-03-22)

[Compare Source](angular/angular@15.2.3...15.2.4)

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [bae6b5ceb1](angular/angular@bae6b5c) | fix | Allow `TestBed.configureTestingModule` to work with recursive cycle of standalone components. ([#&#8203;49473](angular/angular#49473)) |
| [087f4412af](angular/angular@087f441) | fix | more accurate matching of classes during content projection ([#&#8203;48888](angular/angular#48888)) |

#### Special Thanks

Aditya Srinivasan, Alex Rickabaugh, Andrew Scott, Kristiyan Kostadinov, Masaoki Kobayashi, Matthieu Riegler, Paul Gschwendtner, Peter Götz, Thomas Pischke, Virginia Dooley and avmaxim

<!-- CHANGELOG SPLIT MARKER -->

### [`v15.2.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1523-2023-03-16)

[Compare Source](angular/angular@15.2.2...15.2.3)

#### Special Thanks

Alan Agius, Esteban Gehring, Matthieu Riegler and Virginia Dooley

<!-- CHANGELOG SPLIT MARKER -->

### [`v15.2.2`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1522-2023-03-08)

[Compare Source](angular/angular@15.2.1...15.2.2)

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [6207d6f1f0](angular/angular@6207d6f) | fix | add protractor support if protractor imports are detected ([#&#8203;49274](angular/angular#49274)) |

#### Special Thanks

Alan Agius, Andrew Kushnir, Andrew Scott, Kristiyan Kostadinov, Matthieu Riegler, Paul Gschwendtner, Sai Kartheek Bommisetty and Vinit Neogi

<!-- CHANGELOG SPLIT MARKER -->

### [`v15.2.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1521-2023-03-01)

[Compare Source](angular/angular@15.2.0...15.2.1)

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [f0e926074d](angular/angular@f0e9260) | fix | make Location.normalize() return the correct path when the base path contains characters that interfere with regex syntax. ([#&#8203;49181](angular/angular#49181)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [04d8b6c61a](angular/angular@04d8b6c) | fix | do not persist component analysis if template/styles are missing ([#&#8203;49184](angular/angular#49184)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [d60ea6ab5a](angular/angular@d60ea6a) | fix | update zone.js peerDependencies ranges ([#&#8203;49244](angular/angular#49244)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [44d095a61c](angular/angular@44d095a) | fix | avoid migrating the same class multiple times in standalone migration ([#&#8203;49245](angular/angular#49245)) |
| [92b0bda9e4](angular/angular@92b0bda) | fix | delete barrel exports in standalone migration ([#&#8203;49176](angular/angular#49176)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [3062442728](angular/angular@3062442) | fix | add error message when using loadComponent with a NgModule ([#&#8203;49164](angular/angular#49164)) |

#### Special Thanks

Alan Agius, Andrew Kushnir, Aristeidis Bampakos, Craig Spence, Doug Parker, Iván Navarro, Joey Perrott, Kristiyan Kostadinov, Matthieu Riegler, Michael Ziluck, Paul Gschwendtner, Stephanie Tuerk, Vincent and Virginia Dooley

<!-- CHANGELOG SPLIT MARKER -->

</details>

<details>
<summary>angular/angular (zone.js)</summary>

### [`v0.13.0`](angular/angular@zone.js-0.12.0...zone.js-0.13.0)

[Compare Source](angular/angular@zone.js-0.12.0...zone.js-0.13.0)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTQuMSIsInVwZGF0ZWRJblZlciI6IjM1LjI0LjUifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1801
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Location incorrectly handled when served from a base path containing regex-specific characters.
4 participants
@wartab @atscott @AndrewKushnir @jessicajaniuk