fix(runtime): set Response.url and Response.redirected correctly after fetch redirects

[Cypher-CP0]

This Pull Request fixes/closes #5310.

It changes the following:

• Added a redirected: bool field to JsResponse struct in response.rs
• Updated JsResponse::basic() to accept a redirected parameter and store it
• Fixed the Response.redirected getter to return the stored value instead of always returning false
• Updated fetchers.rs to capture the final URL from reqwest before consuming
  the response body, and derive redirected by comparing it to the original request URL
• Updated Response.url to return the final URL after redirects instead of the original request URL
• Added response_redirected_flag test in e2e.rs covering both the redirected and non-redirected cases

[zhuzhu81998]

how does your change align with the TODO comment and spec?

[Cypher-CP0]

TODO was about tracking every hop in the redirect chain, but Response.redirected only needs to know if there was any redirect, not the full list. Since reqwest already follows redirects and exposes the final URL, comparing that to the original request URL gives us exactly what we need. Full URL list tracking could still be a follow-up if needed for other spec requirements.

[zhuzhu81998]

Response.redirected only needs to know if there was any redirect, not the full list

But according to the comment here, spec requires the check to be done via url list, and not a "quick" check like this?

E.g. what if A redirects to B and B redirects back to A? Does your check consider scenario like this?

[Cypher-CP0]

Thanks for catching that!
You're right, A→B→A would break my check. What approach would you recommend, manual redirect handling to build the full URL list, or something simpler?

[zhuzhu81998]

Currently the response only has url, but according to spec and the TODO comment, url list is also required: https://fetch.spec.whatwg.org/#concept-response-url-list . So i guess you should implement that and follow the spec for where to modify it in what way.

But fetch api is in general not complete in boa, so idk how much work that actually might be

[zhuzhu81998]

I am guessing once you have the list, then A -> B -> A would be handled correctly because of the url list containing multiple urls even though the resulting url might be the same as the original.

[HalidOdat]

Yup, we need to keep a list of urls as described in the specification, since we are using reqwest we can create a client that has custom redirect policy, that will collect the url attempts.

    // part of fetcher initialization:
    let url_list = Arc::new(Mutex::new(Vec::new()));
    let url_list_clone = Arc::clone(&history);

    let custom_policy = redirect::Policy::custom(move |attempt| {
        url_list_clone.lock().expect("should not be poisoned").push(attempt.url().clone());
        reqwest::redirect::Policy::default().redirect(attempt)
    });

    let client = Client::builder()
        .redirect(custom_policy)
        .build()?;

    // making the request
    let result = self.client.execute(req);
    
    // clears and takes the list for the previous request.
    let url_list = std::mem::take(&mut *self.url_list.lock().expect("should not be poisoned"));

    let response = result.map_err(JsError::from_rust)?;