Opengrok sends HTTP requests without fragment references

[alighaddar10]

Hello team,

Describe the bug After implementing a single sign-on solution for opengrok on the level of the HAProxy inspired by the issue previously opened on the topic, we noticed a bug in a use-case of opengrok. Whenever a user is opening a link that contains a fragment, for example https://opengrok.example.com/prod/src/build.cpp#getBuild that points to the getBuild() method line, the redirect to authenticate and generate an access token leads the user to https://opengrok.example.com/prod/src/build.cpp thus losing the fragment that points to the specific section. Further investigation lead us to find that the request reaching the proxy already has the fragment reference stripped out, blocking us from preserving it on the proxy level.

To Reproduce Using an SSO secured opengrok instance, try to open a link containing a fragment reference in the URL without having previously authenticated and having a valid access token. It should lead you to the file without the fragment reference still being present in the URL. This behavior is consistent across browsers and even on cURL with follow redirects enabled.

We are using a custom implementation of Single Sign-On authentication and authorization inspired by the following discussion (https://github.com/oracle/opengrok/discussions/4723) with adaptations taylored for our environment and stack.

Expected behavior The URL should retain the fragment reference and send it in the request.

Additional context A suggested solution is to have the reference be a query parameter instead of a fragment reference to be able to receive and parse it to retain the reference when sharing a link to a file. Another solution is to have the fragment reference forcefully retained in the http request if applicable depending on your specific implementation of the client and server side connections.

Please let us know if you need further clarification or if you have another solution in mind to work around this issue. It is blocking for us as it creates an extra barrier for users access opengrok links from documentations which is one of our most common usecases of the service.

Thank you for all your efforts, Sincerely, Ali

[vladak]

Indeed, it seems that the current behavior of various HTTP clients, including browsers, is not to send the fragment identifier in the HTTP request. This means there is no way for it to propagate it through all the authentication redirects, even though the Location HTTP header allows for it in redirects.

The way to pass the fragment identifier is, as you suggest, via a request parameter. This is actually already done for the annotate view to preserve the line numbers using the h parameter: https://github.com/oracle/opengrok/blob/e34493b3497cb7f156be6e033842cdfac9427cb5/opengrok-web/src/main/webapp/js/utils-0.0.47.js#L1677-L1682 https://github.com/oracle/opengrok/blob/e34493b3497cb7f156be6e033842cdfac9427cb5/opengrok-web/src/main/webapp/js/utils-0.0.47.js#L2028-L2036

Given that the piece which extracts the fragment identifier from the h parameter is in domReadyMast (i.e. the consumer), then making this work is just a matter of setting the parameter in window.location, I think.

Even when passing the fragment identifer in the request, the hash links should be preserved to avoid breaking links embedded in bug tracking systems, etc.

Lastly, the hash can consist of two numbers (designating a line range rather than single line) so this needs to be taken into account when encoding it in the URI.

[alighaddar10]

Can you provide us with more details please as to the adaptation needed to make this work? I tested the 'h' parameter and indeed it translates into an anchor. However, the issue is that all the documentations and references use links with anchors as it stands, and it would be very tedious to go over them and change them. Also, kindly note that using the navigate functionality as it stands doesn't generate the parameter but only the anchor, making it even more tedious for users to generate links. Is there a way to enforce the parameter or maybe always translate it? Kindly let us know if there is a solution we can implement that can circumvent these obstacles.

[vladak]

There is no way how to fix this problem for pre-existing links as far as I know due to the reason described above.

For new links, the changes need to happen in two places:

1. in the utils JS code the h parameter needs to be added whenever a line (range) is selected
2. as you noted, the links in the xref needed to be changed as well. This can be done either in the code which produces the xrefs (which would require reindexing from scratch) or alternatively in the JS code to change all these links on the fly (which would probably slow down page rendering a bit). - also, this is not only needed for the Navigate pane links but also for intra-document links (e.g. for locally defined variables and such)

Some work in progress can be seen in https://github.com/vladak/OpenGrok/tree/webapp_frag_ids_via_h_param, notably https://github.com/vladak/OpenGrok/commit/6e27984e66228a01b4b33e7b9421c6f78db2aa37 which appears to do the job for the first item above. I have no capacity to finish this at the moment.

[alighaddar10]

Thank you so much for your efforts and reactivity! We will look into how we can assist with this fix. Please keep us posted when you make progress on this issue on your time of course.

Regarding the pre-existing links, I figured as such and we're brainstorming a way to migrate links to the new format.

Many thanks again.

[vladak]

For the approach with links changed during indexing, I believe most of the links are generated via the methods defined in https://github.com/oracle/opengrok/blob/master/opengrok-indexer/src/main/java/org/opengrok/indexer/analysis/JFlexXrefUtils.java, for example https://github.com/oracle/opengrok/blob/e34493b3497cb7f156be6e033842cdfac9427cb5/opengrok-indexer/src/main/java/org/opengrok/indexer/analysis/JFlexXrefUtils.java#L311-L315

Anyhow, if changed there, this would probably lead to some churn w.r.t. pre-existing xref analyzer tests where xref output is compared to known golden output for given input file.

Further, I am not sure what will happen if these purely anchor links will get converted to full, albeit relative, links. Specifically whether browsers will be able to recognize these links point to the currently displayed page and avoid extra HTTP request.

Also, the line number links themselves (those in the leftmost column) have to be changed.

[alighaddar10]

I see. I was looking through JFlexXrefutils.java, and here I'm concerned that there might be a need to maintain the anchor link as is and append the query parameter to the URL instead of replacing the anchor with the query parameter to avoid major changes being done to the existing logic.

Is this feasible from your side as a way to generate the links? Or do you think the approach should be to adapt entirely to the query parameter instead? I understood that this the considered approach anyways but I just want to make sure.

Also, are there any adaptations needed to JFlexXrefUtils.java#writeSymbol?

You also mentioned that this can be done on the fly but this would cause performance impact. Any idea what impact are we looking at approximately if we go for this approach compared to the reduced cost of implementing such solution?

[alighaddar10]

Hello again,

Is there an approximate timeline for a fix on this issue? we are planning an upgrade of our opengrok instances so we just want to see if it is possible to know if we can get the fix included in our next upgrade.

Thank you for all your efforts.