FuelLabs / FuelLabs/fuel-core

FuelClient::new removes potentially critical path information

Open
#3,176 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
56.8k
Forks
2.9k
Avg merge
2d 9h
Merged PRs (30d)
5

Description

`FuelClient::new` used the `FromStr` implementation of `FuelClient`. This implementation is curious:

```rust
impl FromStr for FuelClient {
type Err = anyhow::Error;

fn from_str(str: &str) -> Result {
let mut raw_url = str.to_string();
if !raw_url.starts_with("http") {
raw_url = format!("http://{raw_url}");
}

let mut url = reqwest::Url::parse(&raw_url)
.map_err(anyhow::Error::msg)
.with_context(|| format!("Invalid fuel-core URL: {str}"))?;
url.set_path("/v1/graphql");

...
}
}
```

Do you see it? `url.set_path` wipes out any path information on the url. Some node providers (quiknode) use the path to store auth information. Of course there is also the option to use headers for quiknode, but there is not an implementation for that here either.

I don't really understand why babysitting the url here is necessary, and it seems to me like this cleaning should just be removed and users be responsible for using correct url paths, however either way this should be rethought.

Contributor guide

Open the contributing guide

Research direction

Start by locating FuelClient::new and the FromStr implementation shown in the issue, then trace how the parsed URL is used for GraphQL requests. Confirm the path is lost and review existing request-header handling; done should preserve supported provider authentication information, with tests covering the chosen URL and header behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, blockchain
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.