Skip to content

fix: url issue for path string for ui tools (playground, voyager, graphiql)#195

Closed
pobisw wants to merge 10 commits into
graphql-dotnet:masterfrom
pobisw:pobisw/Fix_playground_graphiql_path_string_issue_for_schema_fetch
Closed

fix: url issue for path string for ui tools (playground, voyager, graphiql)#195
pobisw wants to merge 10 commits into
graphql-dotnet:masterfrom
pobisw:pobisw/Fix_playground_graphiql_path_string_issue_for_schema_fetch

Conversation

@pobisw
Copy link
Copy Markdown

@pobisw pobisw commented Jan 12, 2019

fixes #184

@pobisw
Copy link
Copy Markdown
Author

pobisw commented Jan 12, 2019

@pekkah @BenjaBobs Can you please take a look at this PR
added the fix for all the 3 UI tool

@pobisw
Copy link
Copy Markdown
Author

pobisw commented Jan 14, 2019

@joemcbride @pekkah please review

@pobisw pobisw changed the title code changes for fixing url issue fix: url issue for path string for ui tools (playground, voyager, graphiql) Jan 16, 2019
@pobisw
Copy link
Copy Markdown
Author

pobisw commented Jan 16, 2019

@pekkah can you please look at this ? this issue is blocking our development

Copy link
Copy Markdown
Member

@joemcbride joemcbride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in the other issue, though this seems to make it so you can’t have your GraphQL endpoint at a different root url, which is a valid scenario. Is there something blocking you from setting that url to the path that you need?

Comment thread src/Ui.GraphiQL/Internal/graphiql.cshtml
@pobisw
Copy link
Copy Markdown
Author

pobisw commented Jan 17, 2019

I mentioned this in the other issue, though this seems to make it so you can’t have your GraphQL endpoint at a different root url, which is a valid scenario. Is there something blocking you from setting that url to the path that you need?

I have replied in the other issue, restating it here, we have multiple int and dev root urls, so we cant set it to a fixed value

@sungam3r sungam3r added bugfix Pull request that fixes a bug needs review labels Feb 24, 2020
@Shane32 Shane32 changed the base branch from develop to master October 26, 2020 23:32
@sungam3r
Copy link
Copy Markdown
Member

@pobisw Thanks for this PR. I will try to review it if you resolve conflicts. Also, could you not change the indentation in the cshtml files so as not to complicate the review?

@sungam3r sungam3r added the needs confirmation The problem is most likely resolved and requires verification by the author label Nov 11, 2020
@Shane32
Copy link
Copy Markdown
Member

Shane32 commented Nov 11, 2020

@pobisw Thanks for this PR. I will try to review it if you resolve conflicts. Also, could you not change the indentation in the cshtml files so as not to complicate the review?

Just FYI, you can turn off whitespace comparison on github's website, @sungam3r

@Shane32
Copy link
Copy Markdown
Member

Shane32 commented Nov 11, 2020

image

@sungam3r
Copy link
Copy Markdown
Member

OK

@pobisw
Copy link
Copy Markdown
Author

pobisw commented Mar 25, 2021

just saw the comments, will make the changes

@sungam3r sungam3r added this to the v5.2 milestone Nov 23, 2021
@Shane32 Shane32 mentioned this pull request Dec 29, 2021
3 tasks
return currentPathName;
}

var strippedCurrentPathName = stripGraphQLUiEndPointFromPathName("@Model.GraphiQLPath");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This injected variable appears to be encoded for HTML instead of for Javascript - correct? Or does Razor detect that it's within a <script> tag and encode it differently?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Razor? No Razor here 🤔 .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cshtml are razor pages and @Model.GraphiQLPath is razor injecting a variable. By default it will format variables with html encoding. So I always use JavaScript encoding as shown below when injecting a variable into a script tag like this. Or encode it as json and then inject it. Or put it into a html tag and read the tag.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cshtml are razor pages

Yes but ... no Razor engine/stuff/handlers/compilers here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just good old string.Replace.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see. Then we should modify the string.Replace encode to javascript-encode the variables for use on the page. Probably not a critical fix as the url is controlled by the setup code, not user input.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


function stripGraphQLUiEndPointFromPathName(graphQlUiEndpoint) {
var currentPathName = window.location.pathname;
var regex = new RegExp(graphQlUiEndpoint + "\/*");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the path name from the model variable is passed directly into a regular expression without encoding. Also seems wrong. Maybe this should not be using regexp at all and just perform a simple replace...

Comment thread src/Ui.GraphiQL/Internal/graphiql.cshtml
@sungam3r
Copy link
Copy Markdown
Member

just saw the comments, will make the changes

@pobisw Thanks for the fix. The main thing here is to resolve conflicts, after which we can continue review.

@sungam3r sungam3r removed this from the v5.2 milestone Jan 1, 2022
@Shane32 Shane32 mentioned this pull request Jul 7, 2022
3 tasks
@Shane32
Copy link
Copy Markdown
Member

Shane32 commented Jul 11, 2022

Replaced by #853

@Shane32 Shane32 closed this Jul 11, 2022
@sungam3r sungam3r removed needs review needs confirmation The problem is most likely resolved and requires verification by the author labels Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

playground is loading schema directly from the baseurl, not considering the string path after first slash

5 participants