withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.57k stars 2.47k forks source link

url encoded dynamic params not properly handled #8516

Closed y-nk closed 1 month ago

y-nk commented 1 year ago

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

rsk700 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

y-nk commented 9 months ago

🤞

castarco commented 8 months ago

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.

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 commented 7 months ago

8j5nlz

ematipico commented 7 months ago

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 commented 7 months ago
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 commented 7 months ago

https://stackblitz.com/edit/withastro-astro-21gk4v?file=src%2Fpages%2F[...path].astro

brokeboiflex commented 7 months ago

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 commented 7 months ago

@brokeboiflex I'm not sure what kind of options you're suggesting, but as you can see from https://github.com/withastro/astro/issues/8516#issuecomment-1999712725, we don't even get right possible non-english URLs.

We have to fix the correct expectations first.

brokeboiflex commented 7 months ago

@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 }, 
  }))
}
y-nk commented 6 months ago

@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 commented 3 months ago

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.

ematipico commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

Fixed in https://github.com/withastro/astro/pull/12079