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

JS: Fix attributes nodes missing an enclosing callable #18973

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 11, 2025

Attribute nodes were missing their enclosing callable, which means the shared data flow library was unable to track flow there. As far as I can tell this only affects Vue.

I'm honestly not sure why DataFlow::XmlAttributeNode exists since we don't seem to use it for anything 😕.

@github-actions github-actions bot added the JS label Mar 11, 2025
@asgerf asgerf force-pushed the js/vue-fix branch 2 times, most recently from b3be197 to 2c8da21 Compare March 11, 2025 11:49
@asgerf asgerf marked this pull request as ready for review March 11, 2025 15:55
@Copilot Copilot bot review requested due to automatic review settings March 11, 2025 15:55
@asgerf asgerf requested a review from a team as a code owner March 11, 2025 15:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

A fix for Vue attribute nodes where attributes were missing an enclosing callable, ensuring that the shared data flow library now properly tracks the flow for the v-html attribute.

  • Updated change note file to document the bug fix.
  • Added inline test comments in the Vue test file to mark source and alert markers.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
javascript/ql/src/change-notes/2025-03-11-vue-fix.md Added change notes for the bug fix
javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/v-html.vue Updated test file with inline comments for source and alert markers

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks sensible, thanks for the clear explanation.

*/
class DataFlowCallable extends TDataFlowCallable {
/** Gets a string representation of this callable. */
string toString() {
result = this.asSourceCallable().toString()
or
result = this.asLibraryCallable()
or
result = this.asFileCallable().toString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I always worry about missing cases like this - this kind of pattern matching feels like a good candidate for a QL-for-QL query :)

@asgerf asgerf merged commit b4016c1 into github:main Mar 12, 2025
14 checks passed
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.

2 participants