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.

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

url encoded dynamic params not properly handled #8516

Closed
1 task done
y-nk opened this issue Sep 12, 2023 · 17 comments
Closed
1 task done

url encoded dynamic params not properly handled #8516

y-nk opened this issue Sep 12, 2023 · 17 comments
Assignees
Labels
5.0 beta Related to the Astro 5.0 beta - P4: important Violate documented behavior or significantly impacts performance (priority) pkg: astro Related to the core `astro` package (scope)

Comments

@y-nk
Copy link
Contributor

y-nk commented Sep 12, 2023

Astro Info

~/projects/withastro-astro-vzlytn
❯ npx astro info
Astro                    v3.0.12
Node                     v16.20.0
System                   Linux (x64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/node
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

i've a url such as /%5Bpage%5D coming from a 3rd party place, but handling this valid url in getStaticPaths breaks the build.

---
export function getStaticPaths() {
  return [{
    params: { path: undefined },
    props: {}
  }, {
    params: { path: '%5Bpage%5D' }, // ← minimal repro
    props: {}
  }]
}
---

<ul>
  <li>hello world</li>
  <li>
    <a href="/%5Bpage%5D">url encoded (should work)</a>
  </li>
  <li>
    <a href="/[page]">url decoded (should not work)</a>
  </li>
</ul>

Error log:

 prerendering static routes 
▶ src/pages/[...path].astro
  ├─ index.html (+7ms)
 error   A `getStaticPaths()` route pattern was matched, but no matching static path was found for requested path `/%5Bpage%5D`.
  Hint:
    Possible dynamic routes being matched: src/pages/[...path].astro.
  Error reference:
    https://docs.astro.build/en/reference/errors/no-matching-static-path-found/
  File:
    /home/projects/withastro-astro-vzlytn/node_modules/astro/dist/core/errors/errors.js:36:5
  Code:
    35 |   setFrame(source, location) {
    > 36 |     this.frame = codeFrame(source, location);
         |     ^
      37 |   }
      38 |   static is(err) {
      39 |     return err.type === "AstroError";

What's the expected result?

it should not matter what chars are in a dynamic param as long as it computes to a valid url, and this one seems to be valid according to new URL('https://example.com/%5Bpage%5D').

image

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-astro-vzlytn?file=src/pages/%5B...path%5D.astro

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Sep 12, 2023
@rsk700
Copy link

rsk700 commented Sep 12, 2023

Have similar issue, maybe related: was trying to upgrade to Astro v3 from v2, I'm generating pages with getStaticPaths.

In Astro v3 the issue I have is it works in production build, on start it works in dev instance, it works on reloads, but if I start navigating website with urls I've got not found for dynamic routes, and error in console says:

02:17:13 PM [getStaticPaths] A `getStaticPaths()` route pattern was matched, but no matching static path was found for requested path `/en/download`.

Possible dynamic routes being matched: src/pages/[lang]/download.astro.
02:17:13 PM [serve]    404                             /en/download

The error appears when I visit same route second time by clicking link to that page.

@spokospace
Copy link

i have the same issue with static page after upgrade to Astro 3. Now I have files like category/subcategory.html instead of category/subcategory/index.html

@yaronovsky
Copy link

Similar issue with ssr in middleware mode, urls with special characters and dynamic url blog/{special characters},
I have an astro middleware and the request is skipped directly to 404

@matthewp matthewp added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged labels Sep 27, 2023
@y-nk
Copy link
Contributor Author

y-nk commented Jan 25, 2024

🤞

@castarco
Copy link
Contributor

castarco commented Feb 22, 2024

EDIT: I guess that my problem is entirely different.

I might be suffering from this same problem. I left a question in the Discord server... but I'm fearing that my problem is due to a bug and not because of anything wrong I did:

https://discord.com/channels/830184174198718474/1210365120832737310

More context on what's happening for my case.

  • Output mode is hybrid
  • We have a route /email/confirm/[validationKey]/
  • Which is handled by a file src/pages/email/confirm/[validationKey].astro.

Now, the contents of this file:

---

export const prerender = false

console.log('A')
const { validationKey } = Astro.params
console.log('B')
console.log(validationKey)

---

<div>
<p>We never get to reach this point</p>
<p>{validationKey}</p>
</div>

When I visit that route... I don't even get a response (not even a 404, but the browser interprets it as a 404)... BUT, I if I visit /email/confirm/ab-cd-ef/, then I can see how the log lines

A
B
ab-cd-ef

are "printed" on the server side... so it is clear that this code is being executed.

Extra:

When I run in verbose mode, I have this output:

  astro:router findPathItemByKey() - Unexpected cache miss looking for "/email/confirm/ab-cd-efg/" +2m
  astro:router findPathItemByKey() - Unexpected cache miss looking for "/email/confirm/ab-cd-efg/" +1ms
A
B
ab-cd-efg

@brokeboiflex

This comment was marked as off-topic.

@ematipico ematipico self-assigned this Mar 15, 2024
@ematipico
Copy link
Member

I am not really sure there's a fix for that. We do decode params because we run them against our route pattern. The route pattern expects a decoded path (the one you're providing via params) because the path could contain non-English characters.

If we decided to NOT decode params, we could break other users's code.

@matthewp
Copy link
Contributor

matthewp commented Mar 15, 2024

example param filename works in dev built in ssg
のツール %E3%81%AE%E3%83%84%E3%83%BC%E3%83%AB %E3%81%AE%E3%83%84%E3%83%BC%E3%83%AB/index.html no yes
のツール のツール のツール/index.html yes yes
[page] %5Bpage%5D ? no no
[page] [page] ? no no

@ematipico
Copy link
Member

@brokeboiflex
Copy link

I am not really sure there's a fix for that. We do decode params because we run them against our route pattern. The route pattern expects a decoded path (the one you're providing via params) because the path could contain non-English characters.

If we decided to NOT decode params, we could break other users's code.

Can't you just make it optional from config?

@ematipico
Copy link
Member

@brokeboiflex
I'm not sure what kind of options you're suggesting, but as you can see from #8516 (comment), we don't even get right possible non-english URLs.

We have to fix the correct expectations first.

@brokeboiflex
Copy link

brokeboiflex commented Mar 20, 2024

@ematipico
I dunno, like an option to not decode params or to support a custom route pattern.
I'm not saying I know your code better than you, I'm just saying that inability to use non english URLs may be a deal breaker for a lot of non english web devs, it certainly almost was for me but I've just worked around it by ditching the non english characters

It's inefficient and dumb af but it got the job done and allowed me to move forward
Maybe it'll help somebody

export async function getStaticPaths() {
  const products = menuSections.flatMap((category) =>
    category.products.map((product) => ({
      categoryName: category.categoryName,
      productName: product.productName,
      productImg: product.productImg,
      productPrice: product.productPrice,
      productDescription: product.productDescription || '',
      variants: product.variants || [],
      sizes: product.sizes || [],
    }))
  )

  return products.map((item) => ({
      params: {
        item: 
          item.productName
            .toLowerCase()
            .replaceAll(' ', '-')
            .replaceAll('ą', 'a')
            .replaceAll('ć', 'c')
            .replaceAll('ę', 'e')
            .replaceAll('ł', 'l')
            .replaceAll('ń', 'n')
            .replaceAll('ó', 'o')
            .replaceAll('ż', 'z')
            .replaceAll('ź', 'z')
      },
      props: { item }, 
  }))
}

@ematipico ematipico added - P4: important Violate documented behavior or significantly impacts performance (priority) pkg: astro Related to the core `astro` package (scope) and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Mar 21, 2024
@y-nk
Copy link
Contributor Author

y-nk commented Apr 18, 2024

@ematipico

We do decode params because we run them against our route pattern. The route pattern expects a decoded path (the one you're providing via params) because the path could contain non-English characters.

If we decided to NOT decode params, we could break other users's code.

it is not a problem of decoded/not-decoded, but imho the framework has a little too much magic on this side (and for good reasons, i can see why you guys did that). but spec wise, %5B represents as much an url-encoded single [ char as much a suite of 3 decoded chars ['%','5','B'] ; each case fulfilling a different but still valid url.

when the 1st occur, all good (from current behavior), but when somebody really meant the 2nd case, then it won't match the input given at getStaticPaths which isn't url encoded and wasn't meant to be (which is the root bug imho). what users pass in params isn't meant to be interpreted as url-encoded ; it's just a plain string param, end of story (as i understand it).

i'm thinking maybe the right move (for this bug and non-english urls) should be not to decode the param for path matching only, mostly because it's a wrong expectation to try matching a freely given string to an url-decoded version of it.

but it doesn't mean we have to break code afterwards. i guess that providing url-decoded params value could make sense as the framework could claim making a pass of sanitization for good DX (and it is always possible to reverse it with a call of encodeURIComponent(Astro.params.foo) in userland, but we can't do it if page not matched)

@ascorbic
Copy link
Contributor

I think the fix would be to change decodeURIComponent here to decodeURI. This would be a breaking change though, so will need to wait til Astro 5. In the meantime a workaround would be to double-encode the params.

@github-project-automation github-project-automation bot moved this to Stage 2: Accepted Proposals, No RFC in Public Roadmap Jul 29, 2024
@ematipico
Copy link
Member

We are going to tackle this in Astro v5.0. It's possible there are going to be some breaking changes, but we want to make sure that we have a predictable encoding/decoding strategy.

We will follow up with some documentation too.

@y-nk
Copy link
Contributor Author

y-nk commented Sep 26, 2024

@ematipico it's greatly appreciated that this issue is not left in the limbos of the great backlog. thanks a lot for the update.

@ematipico
Copy link
Member

Fixed in #12079

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 beta Related to the Astro 5.0 beta - P4: important Violate documented behavior or significantly impacts performance (priority) pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

No branches or pull requests

9 participants