-
Notifications
You must be signed in to change notification settings - Fork 1
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
Hackathon24 #98
Conversation
I can't get this to work with CMW because of three dependency conflicts:
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? |
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 |
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 |
So regarding dependency conflicts: I never touched I was finding 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 |
@mmiller42 Thanks. I got it working. I didn't see those instructions in the README. |
Report too large to display inline |
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Ignoring: Next stepsWhat 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 dependencyTake 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 packageIf 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 riskTo ignore an alert, reply with a comment starting with
|
@SocketSecurity ignore npm/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
I got it working in CMW by removing exact
from the Route
here: https://github.com/smartrent/control.smartrent.com/blob/f497b93655ab7ba4e7d03c80418d7e5e0614f05f/assets/js/react/routers/devtools/private-devtools.tsx#L19
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. |
Contacting GitHub about this. It should not be being blocked anymore. |
@matthew-kerle still not sure what's up here 😅 |
Any update on this? |
Summary of changes
The app should continue to work without clearing localStorage, but a few things might not function quite as intended.
via react-router-dombuilt-in; the newbasename
prop passed to theBadMagic
component defines the Bad Magic URL rootBadMagicProps
Testing
via
example
projectTo test with the included example project, follow the instructions in
README.md
, i.e.:yarn && yarn link
in the root directoryyarn && yarn link badmagic
in the./example
directorycraco
, if you have issues inside theexample
directory, runasdf install && npm i -g yarn
yarn
in the./example-api
directory./example
directory, and./example-api
directory, runyarn start
in CMW
You should also be able to test inside of CMW.
yarn
via homebrew, or it'll cause issues:which yarn
brew uninstall yarn
asdf
should be set up to read the.tool-versions
file in your working directory, cd intobadmagic
, runasdf install
if necessary, and install yarn vianpm i -g yarn
badmagic
. Runyarn -v
and verify that you are running on 1.x (i.e. 1.22.22)yarn && yarn build && yarn pack
yarn pack
command should return a path to the generated tarball file; copy it to your clipboardcontrol.smartrent.com
. Runyarn -v
and verify that the yarn binary is being used from the project (i.e.3.2.0-rc.13
)yarn add badmagic@file:<paste clipboard>
, e.g.yarn add badmagic@/Users/yourname/code/badmagic/badmagic-v0.0.39.tgz
assets/js/react/bundles/devtools/BadMagicClient.tsx
. Add the propbasename="/dev/api"
to the<BadMagic />
component returned byBadMagicClient
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:
yarn build
inside of thebadmagic
repository root so thatmarkdown.min.css
is generated and put inside of./example/public/
cd ./example && yarn start
and verify the example UI loads at http://localhost:3000/dev/apiapps/control_room/lib/control_room_web/templates/development/react/badmagic.html.eex
. Replace theunpkg
URL for BadMagic's stylesheet withhttp://localhost:3000/dev/api/markdown.min.css
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.Most of the features are also testable in the demo app. An API server is included as well.