-
Notifications
You must be signed in to change notification settings - Fork 6
Add proper quotes #22
base: master
Are you sure you want to change the base?
Changes from all commits
2089f28
f8da8bf
3f97dfa
47983ae
ff21e87
a2d4026
4046a9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,8 @@ See the accompanying LICENSE file for terms. | |
| var Parser = require('context-parser').Parser, | ||
| tagAttList = require("./tag-attr-list"), | ||
| derivedState = require('./derived-states.js'), | ||
| xssFilters = require('xss-filters'), | ||
| yubl = require('xss-filters')._privFilters.yubl, | ||
| uriBlacklistFilter = function(s){ return yubl(s.replace(/\x00/g, '%00')); }, | ||
| CssParser = require('css-js'), | ||
| hrefAttribtues = tagAttList.HrefAttributes, | ||
| voidElements = tagAttList.VoidElements; | ||
|
|
@@ -51,14 +52,57 @@ See the accompanying LICENSE file for terms. | |
| return false; | ||
| } | ||
|
|
||
| function addQuoteToValue(attrAction, value) { | ||
| if (attrAction === derivedState.TransitionName.DQ_ATTR) { // collect double quoted value | ||
| return '"' + value + '"'; | ||
| } | ||
| else if (attrAction === derivedState.TransitionName.SQ_ATTR) { // collect single quoted value | ||
| return "'" + value + "'"; | ||
| } | ||
| else /*if (attrAction === derivedState.TransitionName.UQ_ATTR) { // collect unquoted value */{ | ||
| return value; | ||
| } | ||
| } | ||
|
|
||
| function processTransition(prevState, nextState, i) { | ||
| /* jshint validthis: true */ | ||
| /* jshint expr: true */ | ||
| var parser = this.parser, | ||
| action = derivedState.Transitions[prevState][nextState], | ||
| attrAction = action & 0xF, | ||
| idx, tagName, attrValString, openedTag, key, value; | ||
|
|
||
|
|
||
| switch (derivedState.Transitions[prevState][nextState]) { | ||
| // check if tag and/or attr is available for collection | ||
| // collect the values (if any) only when the attr name is allowed | ||
| if (attrAction && contains(this.attributesWhitelist, (key = parser.getAttributeName()))) { | ||
|
|
||
| // collect the attr name only | ||
| // boolean attributes may not have a value | ||
| if (attrAction === derivedState.TransitionName.NV_ATTR) { | ||
| this.attrVals[key] = null; | ||
| } | ||
| // collect both the attr name and value | ||
| else { | ||
| value = parser.getAttributeValue() || ''; | ||
|
|
||
| // store only valid style attribute value | ||
| if (key === 'style') { | ||
| if (this.cssParser.parseCssString(value)) { | ||
| this.attrVals[key] = addQuoteToValue(attrAction, value); | ||
| } | ||
| } | ||
| // apply the blacklist filter for URI attr value | ||
| else if (hrefAttribtues[key]) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we fix the variable name here? s/hrefAttribtues/hrefAttributes ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in #23 |
||
| this.attrVals[key] = addQuoteToValue(attrAction, uriBlacklistFilter(value)); | ||
| } | ||
| else { | ||
| this.attrVals[key] = addQuoteToValue(attrAction, value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // mask out the last 4 bits that represent attr value action | ||
| switch (action & 0xF0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have meaningful macros name instead of 0xF0 here? Something like QUOTE_TYPE_MASK ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is really temporary to minimize changes to the original code to facilitate reviews. :) in fact, I would like to drop using switch but make it like you bet which one is faster? :) |
||
|
|
||
| case derivedState.TransitionName.WITHIN_DATA: | ||
| this.output += parser.input[i]; | ||
|
|
@@ -87,69 +131,31 @@ See the accompanying LICENSE file for terms. | |
| } | ||
| else { | ||
| // - void elements only have a start tag; end tags must not be specified for void elements. | ||
| this.hasSelfClosing = this.hasSelfClosing || voidElements[tagName]; | ||
| this.hasSelfClosing = /*this.hasSelfClosing ||*/ voidElements[tagName]; | ||
|
|
||
| // push the tagName into the openedTags stack if not found: | ||
| // - a self-closing tag or a void element | ||
| this.config.enableTagBalancing && !this.hasSelfClosing && this.openedTags.push(tagName); | ||
|
|
||
| if (prevState === 35 || | ||
| prevState === 36 || | ||
| prevState === 40) { | ||
| this.attrVals[parser.getAttributeName()] = parser.getAttributeValue(); | ||
| } | ||
|
|
||
| // output all allowed attr names and values | ||
| attrValString = ''; | ||
| for (key in this.attrVals) { | ||
| if (contains(this.attributesWhitelist, key)) { | ||
| value = this.attrVals[key]; | ||
|
|
||
| if (key === "style") { // TODO: move style to a const | ||
| if (value === null) { | ||
| attrValString += ' ' + key + '=""'; | ||
| } | ||
| else if (this.cssParser.parseCssString(value)) { | ||
| attrValString += ' ' + key + '="' + value + '"'; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| attrValString += ' ' + key; | ||
| if (value !== null) { | ||
| attrValString += '="' + (hrefAttribtues[key] ? xssFilters.uriInDoubleQuotedAttr(decodeURI(value)) : value) + '"'; | ||
| } | ||
| } | ||
| value = this.attrVals[key]; | ||
| attrValString += (value === null) ? ' ' + key : ' ' + key + '=' + value; | ||
| } | ||
|
|
||
| // handle self-closing tags | ||
| this.output += '<' + tagName + attrValString + (this.hasSelfClosing ? ' />' : '>'); | ||
|
|
||
| } | ||
| } | ||
| // reinitialize once tag has been written to output | ||
| this.attrVals = {}; | ||
| this.hasSelfClosing = 0; | ||
| break; | ||
|
|
||
| case derivedState.TransitionName.ATTR_TO_AFTER_ATTR: | ||
| this.attrVals[parser.getAttributeName()] = null; | ||
| // this.hasSelfClosing = false; | ||
| break; | ||
|
|
||
| case derivedState.TransitionName.ATTR_VAL_TO_AFTER_ATTR_VAL: | ||
| this.attrVals[parser.getAttributeName()] = parser.getAttributeValue() || ''; | ||
| break; | ||
|
|
||
| //case derivedState.TransitionName.TAG_OPEN_TO_MARKUP_OPEN: | ||
| // this.output += "<" + parser.input[i]; | ||
| // break; | ||
|
|
||
| case derivedState.TransitionName.TO_SELF_CLOSING_START: | ||
| // boolean attributes may not have a value | ||
| if (prevState === 35) { | ||
| this.attrVals[parser.getAttributeName()] = null; | ||
| } | ||
| this.hasSelfClosing = 1; | ||
| break; | ||
| // case derivedState.TransitionName.TO_SELF_CLOSING_START: | ||
| // this.hasSelfClosing = true; | ||
| // break; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -159,7 +165,7 @@ See the accompanying LICENSE file for terms. | |
| that.output = ''; | ||
| that.openedTags = []; | ||
| that.attrVals = {}; | ||
| that.hasSelfClosing = 0; | ||
| // that.hasSelfClosing = false; | ||
| that.parser.reset(); | ||
| that.parser.contextualize(data); | ||
|
|
||
|
|
||
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.
can we document what each bit/byte is reserved for? That'll help us when we add new actions going forward.
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 believe its documented a few lines later. e.g.,
DerivedState.TransitionName.DQ_ATTR = 0x01;, andDerivedState.TransitionName.WITHIN_DATA = 0x10;with comments therecan you pls share how it should be better improved? :)
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.
on a second thought, are you expecting something like 0x01 refers to the least significant bit (from right to left, first bit), 0x02 refers to the second, 0x04 refers to the third, and 0x08 refers to the fourth one, so and so forth?