Skip to content

Conversation

@leman31
Copy link

@leman31 leman31 commented Jan 17, 2025

No description provided.

@mciprian13
Copy link

@lutzroeder Can you please have another look? We are available to answer your questions and requests related to this PR. Thank you!

@lutzroeder lutzroeder force-pushed the main branch 5 times, most recently from 3a62144 to 1c38361 Compare March 30, 2025 16:32
@mciprian13
Copy link

@lutzroeder Kind reminder to help us, based on your availability, to review and land this feature which is important to us.

@lutzroeder lutzroeder force-pushed the main branch 2 times, most recently from c0a8d0d to 94c2a0e Compare April 7, 2025 01:23
lutzroeder added a commit that referenced this pull request Apr 11, 2025
@lutzroeder
Copy link
Owner

lutzroeder commented Apr 11, 2025

Hi @mciprian13 @leman31 — to help move this forward:

  • Let's revert all security-related changes, including the use of unsafe-inline and unsafe-eval.

  • The idea of enabling actions is interesting. However, allowing arbitrary executables to be invoked based on a metrics file introduces significant security concerns — e.g., could a metrics file be crafted to execute malicious code? Also, referencing specific executables isn't portable across platforms. Have you considered using protocol handlers instead?

  • The current pull request modifies and cleans HTML directly, rather than building on the existing data model and sidebar integration. I pushed a change to main that extends the current metrics implementation to support external .metrics.json files using the existing sidebar logic. Would it be possible to shift to that approach as a baseline?

See #1240 for examples. If you open the attached model files and drag a .json file into the view, its metrics will appear in the sidebar alongside built-in and model-derived metrics.

lutzroeder added a commit that referenced this pull request Apr 11, 2025
lutzroeder added a commit that referenced this pull request Apr 12, 2025
@lutzroeder lutzroeder force-pushed the main branch 2 times, most recently from 6e60e95 to df7f56d Compare April 13, 2025 15:58
@mciprian13
Copy link

Hi @mciprian13 @leman31 — to help move this forward:

  • Let's revert all security-related changes, including the use of unsafe-inline and unsafe-eval.
  • The idea of enabling actions is interesting. However, allowing arbitrary executables to be invoked based on a metrics file introduces significant security concerns — e.g., could a metrics file be crafted to execute malicious code? Also, referencing specific executables isn't portable across platforms. Have you considered using protocol handlers instead?
  • The current pull request modifies and cleans HTML directly, rather than building on the existing data model and sidebar integration. I pushed a change to main that extends the current metrics implementation to support external .metrics.json files using the existing sidebar logic. Would it be possible to shift to that approach as a baseline?

For example, if you open the attached model file and drag a .metrics.json file into the view, its metrics will appear in the sidebar alongside built-in and model-derived metrics.

mnist.metrics.zip model.metrics.zip

To your points:

  1. We can revert those changes although we will have to reevaluate to see whether some features are impacted or not.
  2. Having actions by running executables/commands is a very powerful way to add all sort of functionality. For us the security aspect is irrelevant because we are creating the meta files and we are consuming the meta files. The meta files are ways to add extra information/functionality to Netron from outside. This is good enough for us.
  3. Having the same meta in JSON format is a small change but changing the way the same information is imported in the tool is a very significant change and I am afraid we don't have time for such a big change. We will have to evaluate the effort needed but if it is too large we will have to drop the path of upstreaming this.

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.

3 participants