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

Hackathon24 #98

Closed
wants to merge 13 commits into from
Closed

Hackathon24 #98

wants to merge 13 commits into from

Conversation

mmiller42
Copy link
Contributor

@mmiller42 mmiller42 commented Jul 2, 2024

Summary of changes

The app should continue to work without clearing localStorage, but a few things might not function quite as intended.

  • Added routing via react-router-dom built-in; the new basename prop passed to the BadMagic component defines the Bad Magic URL root
  • Highlighted active item in nav sidebar
  • JSON keys are in alphabetical order
  • Syntax highlighting is applied to code blocks in Markdown docs
  • Search text is persisted to localStorage
  • Added a button to clear history (both at an individual endpoint level as well as globally)
  • Fixed all the eslint warnings
  • Created a config provider to store the given BadMagicProps
  • Fixed various bugs where the incorrect route could be "matched" for a HistoricResponse (uniqueness requires workspace + path + http method + endpoint name)
  • Added deprecated label to the nav sidebar
  • Routes are sorted in nav somewhat logically

Testing

via example project

To test with the included example project, follow the instructions in README.md, i.e.:

  1. Run yarn && yarn link in the root directory
  2. Run yarn && yarn link badmagic in the ./example directory
    1. Note that I had to use a different nodejs version because of craco, if you have issues inside the example directory, run asdf install && npm i -g yarn
  3. Run yarn in the ./example-api directory
  4. In the root directory, ./example directory, and ./example-api directory, run yarn start

in CMW

You should also be able to test inside of CMW.

  1. First, verify you have not installed yarn via homebrew, or it'll cause issues: which yarn
    1. If so, brew uninstall yarn
    2. Since asdf should be set up to read the .tool-versions file in your working directory, cd into badmagic, run asdf install if necessary, and install yarn via npm i -g yarn
  2. cd into the root directory of badmagic. Run yarn -v and verify that you are running on 1.x (i.e. 1.22.22)
  3. Run yarn && yarn build && yarn pack
  4. The yarn pack command should return a path to the generated tarball file; copy it to your clipboard
  5. cd into the root directory of control.smartrent.com. Run yarn -v and verify that the yarn binary is being used from the project (i.e. 3.2.0-rc.13)
  6. Run yarn add badmagic@file:<paste clipboard>, e.g. yarn add badmagic@/Users/yourname/code/badmagic/badmagic-v0.0.39.tgz
  7. Open assets/js/react/bundles/devtools/BadMagicClient.tsx. Add the prop basename="/dev/api" to the <BadMagic /> component returned by BadMagicClient

Markdown CSS

I made some slight adjustments to the Markdown CSS file as well, though due to the fact that the syntax highlighter tries to automatically infer the language even without a language tag, I don't think you'll actually see the CSS changes because they were only affecting some unhighlighted code elements. Regardless, if you want to also test it with the latest CSS file:

  1. Make sure you've run yarn build inside of the badmagic repository root so that markdown.min.css is generated and put inside of ./example/public/
  2. Make sure the example UI server is running, i.e. cd ./example && yarn start and verify the example UI loads at http://localhost:3000/dev/api
    1. We only need this because the example UI server also serves the stylesheet
  3. In CMW, open apps/control_room/lib/control_room_web/templates/development/react/badmagic.html.eex. Replace the unpkg URL for BadMagic's stylesheet with http://localhost:3000/dev/api/markdown.min.css
  4. Open apps/control_room/lib/control_room_web/plugs/secure_browser_headers.ex. Find the "style-src" list and append the "http://localhost:3000" string to the list of allowed domains.
  5. Run CMW as normal!

Most of the features are also testable in the demo app. An API server is included as well.

@justincy
Copy link

justincy commented Jul 3, 2024

I can't get this to work with CMW because of three dependency conflicts:

  • axios@npm:0.24.0 conflicts with parent dependency axios@npm:0.26.1
  • react-native-svg@npm:13.14.0 conflicts with parent dependency react-native-svg@npm:12.3.0
  • react-router-dom@npm:6.24.1 conflicts with parent dependency react-router-dom@npm:5.3.4

The axios and react-native-svg conflicts will be trivial to resolve. It'd be a lot of work for CMW to upgrade to react-router-dom. Any chance we could use v5 here instead? Do we have any apps already using v6?

@justincy
Copy link

justincy commented Jul 3, 2024

I haven't been able to get the example working either. This is what I get after updating the badmagic dep on the client to point to ../ so it pulls the PR version instead of the latest published version.

image

@mmiller42
Copy link
Contributor Author

So I'm going to try and see if I can get a full set of steps to install this locally, because there shouldn't be a dependency conflict issue, but I think that's creeping up because of how the badmagic module's dependencies are resolved specifically when trying to link the project locally rather than the normal install process. Because these conflicts shouldn't be conflicts; they are incompatible semver ranges, so both versions of the given packages should technically be installed. This was working for me locally when I was running my copy of Bad Magic in CMW and was pretty easy to confirm, because react-router-dom v4 has a completely different API, but both worked.

I'm probably not going to have time to pick this back up for a while

@mmiller42
Copy link
Contributor Author

I'm not following your screenshot either though... I'm not sure I understand why the example built into the repo wouldn't be working... is that after following the instructions in the README, using yarn link and yarn link badmagic?

@mmiller42
Copy link
Contributor Author

mmiller42 commented Jul 3, 2024

So regarding dependency conflicts:

I never touched axios or react-native-svg, so both versions of these packages should be getting installed and bundled, which I would expect to already be happening.

I was finding react-router-dom to be a trickier situation. In practice, it was actually running just fine, with the CMW app using react-router-dom@5 and the Bad Magic component rendering with react-router-dom@6. TypeScript, on the other hand, was very unhappy with the situation. I'm not sure if this was an issue with the method I was using to "link" and/or install the badmagic package locally. But basically the problem is, react-router-dom@5 does not bundle TypeScript declarations, so we have @types/react-router-dom installed as well. react-router-dom@6, however, includes the declarations with the package. It seems that because two versions of react-router-dom are installed in the root project (CMW), TypeScript was "preferring" to get the embedded declarations from react-router-dom@6 and ignoring @types/react-router-dom. I tried a few tricks to override this without much luck.

I don't really want to install react-router-dom@5 into the badmagic project, since a newer greenfield app that wants to use Bad Magic is likely going to install the latest React Router. I also can't just make react-router-dom@* one of the peerDependencies either because of the breaking changes. Considering how little routing is actually going on in the app, and that when we added the "copy link to clipboard" feature we didn't use any library at all, I'm considering just removing react-router-dom entirely and just inline what we need.

@justincy
Copy link

justincy commented Jul 3, 2024

@mmiller42 Thanks. I got it working. I didn't see those instructions in the README.

image

@justincy
Copy link

justincy commented Jul 3, 2024

I also managed to get it running in CMW (I must've had a bad cache somewhere previously). But I think the TS issues are going to be a problem with CMW adoption.

image

Copy link

socket-security bot commented Jul 7, 2024

Report too large to display inline

View full report↗︎

Copy link

socket-security bot commented Jul 7, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Shell access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Shell access npm/[email protected] ⚠︎
AI warning npm/@pmmmwh/[email protected]
  • Notes: The code contains multiple anomalies including hardcoded environment variable, unusual conditions, potential backdoor, dynamic function creation, and dynamic module injections, which indicate a high probability of malicious behavior or security risks.
  • Confidence: 1.00
  • Severity: 0.60
🚫
Shell access npm/[email protected] ⚠︎
Shell access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎
Network access npm/[email protected] ⚠︎

Ignoring: npm/[email protected]

View full report↗︎

Next steps

What is shell access?

This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code.

Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced.

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

What is an AI-detected potential code anomaly?

AI has identified unusual behaviors that may pose a security risk.

An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@mmiller42
Copy link
Contributor Author

@SocketSecurity ignore npm/[email protected]

@mmiller42 mmiller42 requested a review from justincy July 8, 2024 01:02
Copy link

@justincy justincy left a comment

Choose a reason for hiding this comment

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

🎉

image

Copy link

@justincy justincy left a comment

Choose a reason for hiding this comment

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

I have to take that back. In CMW, nothing renders after navigating to a route and then reloading the page. I have basename="/dev/api" on the BadMagic component. I'll look into it:

image

@mmiller42
Copy link
Contributor Author

mmiller42 commented Jul 11, 2024

@justincy Hold on I had a fix for this, I know it's very simple, but there was a required code change to CMW I believe and I forgot to mention it in the PR. I'll follow up here in a minute

EDIT: Something else must have broken in my recent changes -- you can fix the routing behavior in CMW with a simple change to apps/control_room/lib/control_room_web/routes/development.ex:

    scope "/api" do
      pipe_through(:badmagic_layout)

      get("/", Development.ReactController, :api)
      match(:*, "/*path", Development.ReactController, :api)
    end

However, when refreshing you'll get a blank page -- it IS rendering BadMagic though, so there's a bug somewhere with matching the route 😅

EDIT 2: I think it actually is working now? Not sure, I think I had a caching issue... @justincy can you try adding this code change and testing it again?

@mmiller42 mmiller42 requested a review from justincy July 15, 2024 17:34
Copy link

@justincy justincy left a comment

Choose a reason for hiding this comment

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

@mmiller42
Copy link
Contributor Author

Thanks for the approvals! Unfortunately merging this PR is blocked by this "CodeQL" test and I have no ay around it. I am not sure who has administrative access to the repo, nor how to publish a new version if I did merge.

@mmiller42 mmiller42 closed this Jul 18, 2024
@mmiller42 mmiller42 reopened this Jul 18, 2024
@mmiller42 mmiller42 closed this Jul 18, 2024
@mmiller42 mmiller42 reopened this Jul 18, 2024
@matthew-kerle
Copy link

Contacting GitHub about this. It should not be being blocked anymore.

@jollyjerr jollyjerr removed their request for review July 18, 2024 16:47
@mmiller42 mmiller42 closed this Jul 22, 2024
@mmiller42 mmiller42 reopened this Jul 22, 2024
@mmiller42
Copy link
Contributor Author

@matthew-kerle still not sure what's up here 😅

@justincy
Copy link

Any update on this?

@mmiller42 mmiller42 closed this Aug 16, 2024
@mmiller42 mmiller42 deleted the hackathon24 branch August 16, 2024 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants