Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEA-2863: Expose debug transport URL when possible #64

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

evanweible-wf
Copy link
Contributor

@evanweible-wf evanweible-wf commented Nov 1, 2023

Motivation

For debugging purposes, it would be helpful to have access to the full URL used by the underlying SockJS transport (the one that includes the selected protocol and session ID). This is currently not exposed publicly anywhere.

Changes

  • Update the unminified sockjs.js file wrapped by this library to set transportUrl on the SockJS client instance.
  • Update the SockJSOpenEvent Dart type to include a debugUrl field that gets set to the new transportUrl field

@aviary2-wf
Copy link

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(1) Security relevant changes were detected
  • Watched file lib/sockjs.js modified
  • Action Items

    • Obtain a security review; reviewer should pay special attention to insights listed above
    • Verify aviary.yaml coverage of security relevant code

    Questions or Comments? Reach out on Slack: #support-infosec.

    @@ -29,4 +29,4 @@ echo.on('connection', function(conn) {

    var server = http.createServer();
    echo.installHandlers(server, {prefix:'/echo'});
    server.listen(9999, '0.0.0.0');
    server.listen(9009, '0.0.0.0');
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    The dart analysis server's diagnostics run on this port, so I changed it.

    lib/sockjs.js Outdated
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    This gets auto-collapsed by Github but it's the change that should be reviewed :)

    yuanmwang-wf
    yuanmwang-wf previously approved these changes Nov 1, 2023
    @corwinsheahan-wf
    Copy link

    security +1

    • The debugURL is not sensitive and is already available in the browser console.

    @rmconsole2-wf rmconsole2-wf changed the title Expose debug transport URL when possible FEA-2863 Expose debug transport URL when possible Nov 2, 2023
    @bender-wk bender-wk changed the title FEA-2863 Expose debug transport URL when possible FEA-2863: Expose debug transport URL when possible Nov 2, 2023
    @evanweible-wf
    Copy link
    Contributor Author

    QA +1

    • Tested with our app and verified that the debug URL is set correctly

    @Workiva/release-management-p

    Copy link
    Contributor

    @rmconsole-wf rmconsole-wf left a comment

    Choose a reason for hiding this comment

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

    +1 from RM

    @rmconsole2-wf rmconsole2-wf merged commit 11cdc7d into master Nov 2, 2023
    6 checks passed
    @rmconsole2-wf rmconsole2-wf deleted the expose-debug-transport-url branch November 2, 2023 21:02
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    6 participants