doc($sce): Overhaul the $sce service's documentation. #15735

rjamet · 2017-02-21T14:20:56Z

A big docs update around the $sce: there's a lot of content in there that's often misunderstood, and some of the documentation starts to get really old too. Plus some minor falsehoods in there... Overall, this isn't a major refactoring, but there's a lot of smaller changes.

cc @mprobst for another set of eyes on the contents.

Does this PR introduce a breaking change?
Nope, it's a doc-only CL.

Other information:

I haven't tested how the doc renders, as I couldn't find a way to do so ?

A big docs update around the $sce: there's a lot of content in there that's often misunderstood, and some of the documentation starts to get really old too.

mprobst · 2017-02-21T15:36:37Z

src/ng/sce.js

-     * attribute interpolation, any dom event binding attribute interpolation
-     * such as for onclick,  etc.) that uses the provided value.
-     * See {@link ng.$sce $sce} for enabling strict contextual escaping.
+     * Marks the parameter as trusted for the specified context, and returns the


This is misleading - this doesn't change the parameter at all, but the language suggests it: "marks the parameter...". Maybe "Returns a version of the parameter that is trusted for the specified context..." or so?

Yup, right, my formulation might imply that there's a global list of trusted stuff somewhere. Changed to "Returns a trusted representation of the parameter for the specified context. "

mprobst · 2017-02-21T15:36:51Z

src/ng/sce.js

-     * such as for onclick,  etc.) that uses the provided value.
-     * See {@link ng.$sce $sce} for enabling strict contextual escaping.
+     * Marks the parameter as trusted for the specified context, and returns the
+     * trusted representation of it. This trusted object should later on be


s/should/will/?

mprobst · 2017-02-21T15:37:33Z

src/ng/sce.js

-     * See {@link ng.$sce $sce} for enabling strict contextual escaping.
+     * Marks the parameter as trusted for the specified context, and returns the
+     * trusted representation of it. This trusted object should later on be
+     * used as-is, without any security check, by sinks that use this context.


I'm not sure users will undestand what sinks are in this context. Maybe just say "Angular", or Angular's templating engine, or so? It's less precise, but much closer to the mental model most users have.

I've replaced it by "used as is without any security checks by bindings or directives that require this security context."

(I'd like to keep the notion that it's not automagical and that modules are on the hook for their contexts)

mprobst · 2017-02-21T15:38:32Z

src/ng/sce.js

+     * For instance, marking a string as trusted for the $sce.HTML context will
+     * entirely bypass the potential $sanitize call in corresponding $sce.HTML
+     * sinks, such as an ng-bind-html directive, while using the same value
+     * passed as a string will cause it to be sanitized.


It might be worth calling out that calling this method does not need to be called for most use cases where HTML is interpolated, as sanitization is fine?

Added "Note that in most cases you won't need to call trustAsHtml: if you have the sanitizer loaded, passing the value itself will render all HTML that does not pose a security risk."

mprobst · 2017-02-21T15:42:27Z

src/ng/sce.js

+     * for general documentation about strict contextual escaping.
+     *
+     * @param {string} type The kind of context in which this value is safe
+     *   for use.  e.g. url, resourceUrl, html, js and css.


nit +4 indent

Should I do that for the whole file, while I'm at it? Do you have a style guide for doc?

Please do. I don't think we have a style guide in Angular, but +4 matches Google JS style and what you generally do in other languages.

Ok, I'll do that. 80 or 100 cols, btw? (edit: CONTRIBUTING.md says 100, I'll go with that)

mprobst · 2017-02-21T15:43:14Z

src/ng/sce.js

-     * Disabling auto-escaping is extremely dangerous, it usually creates a Cross Site Scripting
-     * (XSS) vulnerability in your application.
-     * </div>
+     * In practice, there's several cases. When given a string, then this should


s/should/will/, or just this runs checks and sanitization....

mprobst · 2017-02-21T15:46:02Z

src/ng/sce.js

- * bindings.  (HTML is just one example of a context where rendering user controlled input creates
- * security vulnerabilities.)
+ * disabled, this application allows the user to render arbitrary HTML into the DIV, which would
+ * be an XSS bug. In a more realistic example, one may be rendering user comments, blog articles,


maybe XSS security bug or XSS security hole in your application? Users might not know what this means, being explicit might make them look it up.

mprobst · 2017-02-21T16:17:20Z

src/ng/sce.js

-     *     return value of {@link ng.$sce#trustAs $sce.trustAs}.)
+     * @param {*} value The value to mark as trusted for URL context.
+     * @returns {*} A wrapped version of value that can be used as a trusted variant
+     *   of your `value` in URL context (currently unused.)


(currently untrusted) is a bit misleading, maybe used as a trusted variant of your previously untrusted value?

it's unused, not untrusted. Fixed by saying more explicitly that the context is unused so the corresponding trustAs has no reason to be called.

rjamet · 2017-02-22T13:53:41Z

Some more formatting fixes after your comments:

+4 spaces for indents
100 col everywhere
s/@returns/@return/
I uniformized context names : it's not "x for a kind of context" anymore, just "x for context", and always refer to them with the form $sce.HTML
a bunch of backticks here and there

mprobst · 2017-02-22T14:11:09Z

LGTM.

gkalpak · 2017-02-28T08:57:28Z

src/ng/sce.js

   *
   * @return {Array} the currently set whitelist array.
   *
   * The **default value** when no whitelist has been explicitly set is `['self']` allowing only
   * same origin resource requests.
   *
+   *     <div class="alert alert-warning">
+   *     **Note:** the default whitelist of 'self' is not recommanded if your app shares its origin


recommanded --> recommended

While you are at it, can you indent the preceeding paragraph as well for consistency?

gkalpak · 2017-02-28T09:01:59Z

src/ng/sce.js

-     * @param {*} value The value that that should be considered trusted/safe.
-     * @returns {*} A value that can be used to stand in for the provided `value` in places
-     * where AngularJS expects a $sce.trustAs() return value.
+     * @param {*} value The value that that should be considered trusted.


that that --> that

gkalpak · 2017-02-28T09:04:05Z

src/ng/sce.js

-     * Disabling auto-escaping is extremely dangerous, it usually creates a Cross Site Scripting
-     * (XSS) vulnerability in your application.
-     * </div>
+     * In practice, there's several cases. When given a string, this run checks and sanitization to


there's --> there are
this run --> it runs

gkalpak · 2017-02-28T09:04:48Z

src/ng/sce.js

-     * </div>
+     * In practice, there's several cases. When given a string, this run checks and sanitization to
+     * make it safe without prior assumptions. When given the result of a {@link
+     * ng.$sceDelegate#trustAs `$sceDelegate.trustAs`} call, this returns the originally supplied


this --> it (we generally try to avoid "this" as it can be confusing)

done (plus another this above clarified in this function)

gkalpak · 2017-02-28T09:05:52Z

src/ng/sce.js

-     * @returns {*} The value the was originally provided to {@link ng.$sceDelegate#trustAs
-     *     `$sceDelegate.trustAs`} if valid in this context.  Otherwise, throws an exception.
+     *     `$sceDelegate.trustAs`} call, or anything else (which will not be considered trusted.)
+     * @return {*} A version of value that's safe to use in the given context, or throws an


of value --> of the value

gkalpak · 2017-02-28T09:14:19Z

src/ng/sce.js

+ * context.  That trust is formalized with a function call. This means that as a developer, you
+ * can assume all untrusted bindings are safe. Then, to audit your code for binding security issues,
+ * you just need to ensure the values you mark as trusted indeed are safe - because they were
+ * received from your server, sanitized by your library, etc.. You can organize your codebase to


etc.. --> etc.

gkalpak · 2017-02-28T09:18:20Z

src/ng/sce.js

- * through {@link ng.$sce#getTrusted $sce.getTrusted}.  SCE doesn't play a role here.
+ * call `$sce.trustAs` on them (e.g.
+ * `<div ng-bind-html="'<b>implicitly trusted</b>'"></div>`) just works. The `$sceDelegate` will
+ * also use the `$sanitize` injectable if it is available when binding untrusted values to


injectable --> service (It would be clearer to the reader, as this is the term we use throughout the docs.)

gkalpak · 2017-02-28T09:19:47Z

src/ng/sce.js

+     *
+     * @param {*} value The value to mark as trusted for `$sce.CSS` context.
+     * @return {*} A wrapped version of value that can be used as a trusted variant
+     *     of your `value` in CSS context. This context is currently unused, so there are almost no


CSS --> `$sce.CSS` (?)

I missed that one. Fixed.

gkalpak · 2017-02-28T09:20:53Z

src/ng/sce.js

+     * takes any input, and either returns a value that's safe to use in the specified context,
+     * or throws an exception. This function is aware of trusted values created by the `trustAs`
+     * function and its shorthands, and when contexts are appropriate, returns the unwrapped value
+     * as-is. Finally, this function can also throw when there is no way to turn maybeTrusted in a


maybeTrusted --> `maybeTrusted`

gkalpak · 2017-02-28T09:21:25Z

src/ng/sce.js

-     *              Otherwise, throws an exception.
+     * @param {string} type The context in which this value is to be used.
+     * @param {*} maybeTrusted The result of a prior {@link ng.$sce#trustAs `$sce.trustAs`} call.
+     * @return {*} The value the was originally provided to {@link ng.$sce#trustAs `$sce.trustAs`}


the was --> that was

gkalpak · 2017-02-28T09:29:16Z

BTW, regarding the 4-space indentation on @param or @return descriptions: I think that is some cases (e.g. when there are bullet-points or empty lines) dgeni will recongnize this indended blocks as code blocks and wrap them in <pre> tags. Have you checked the rendered output?

rjamet · 2017-03-03T14:54:29Z

I don't know how to render these so far, so everything is blind edits... I'll look into dgeni.

gkalpak · 2017-03-04T22:12:12Z

My only concern is whether the 4-space indentations make some blocks incorrectly rendered as code blocks. Other than that it LGTM.

@rjamet, you can see the rendered output with the folloing commands:

grunt package   # May take a while :)
grunt webserver

The open http://localhost:8000/build/docs/.

(After you have run grunt package once, you can run grunt docs (to only update the docs) and reload your browser to see the changes.)

rjamet · 2017-03-15T14:31:47Z

Your fears are justified, that parses paragraphs in param/return blocks as code blocks if there's an empty line.

I'm tempted to keep the +4 indentation, and work around this by either two spaces + simple line break (=  in markdown), or . Would that be fine? It lets us be consistent, even if it's not that pretty: the issue is then to know if anything else consumes this. BR loses paragraphs, paragraphs add markup that might not make sense in IDEs.

I spotted a few unrelated issues otherwise, I'll bundle them as part of the next fix.

gkalpak · 2017-03-20T14:23:19Z

Either way works for me, as long as the output looks the way it should.

@param

Some capitalization was wrong, formatting fixes for +4 indentation on @param tags, and uniformize some params descriptions on the same functions (scedelegate / sce).

rjamet · 2017-03-21T13:56:58Z

Ok, fixed:

capitalization of the return sections
some sce/scedelegate uniformization between getTrusted parameters
one "js" that should have been $sce.JS
default values for the resource url white/blacklists should be described in description, not in return
+4 indentation everywhere it makes sense,  to keep paragraphs in there, except on the parseAs functions where I kept the +3 because +4-indented lists resist any dirty hack I could find :/

I checked all the $sce doc output, it's all sensible at least. I kept paragraphs where they made sense.

gkalpak

I trust you that it looks good when rendered 😃
(You still need to fix the linting issue Travis complains about.)

rjamet · 2017-03-21T14:51:47Z

Oops, sorry, I missed it. Fixed.

A big docs update around `$sce`: There is a lot of content in there that is often misunderstood, and some of the documentation starts to get really old too. Also fixed capitalization, formatting, indentation and uniformized `@param` descriptions. Closes #15735

doc($sce): Overhaul the $sce service's documentation.

d346a00

A big docs update around the $sce: there's a lot of content in there that's often misunderstood, and some of the documentation starts to get really old too.

googlebot added the cla: yes label Feb 21, 2017

mprobst reviewed Feb 21, 2017

View reviewed changes

doc($sce): adress comments, plus more formatting fixes

5e6a040

mprobst approved these changes Feb 22, 2017

View reviewed changes

gkalpak suggested changes Feb 28, 2017

View reviewed changes

gkalpak added component: misc core type: docs labels Feb 28, 2017

gkalpak added this to the Backlog milestone Feb 28, 2017

doc($sce): Adress gkalpak's comments.

ecefd07

doc($sce): more minor formatting/content fixes.

2d437bd

Some capitalization was wrong, formatting fixes for +4 indentation on @param tags, and uniformize some params descriptions on the same functions (scedelegate / sce).

doc($sce): add missing paragraph in resourceUrlBlacklist

90fec6a

gkalpak approved these changes Mar 21, 2017

View reviewed changes

doc($sce): trailing space

ac1c9f4

gkalpak closed this in 0d9d57d Mar 21, 2017

doc($sce): Overhaul the $sce service's documentation. #15735

doc($sce): Overhaul the $sce service's documentation. #15735

Conversation

rjamet commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

rjamet commented Feb 22, 2017

mprobst commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Feb 28, 2017

rjamet commented Mar 3, 2017

gkalpak commented Mar 4, 2017

rjamet commented Mar 15, 2017

gkalpak commented Mar 20, 2017

rjamet commented Mar 21, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

rjamet commented Mar 21, 2017

rjamet Feb 22, 2017 •

edited

Loading

rjamet Feb 22, 2017 •

edited

Loading

rjamet Feb 22, 2017 •

edited

Loading

rjamet commented Mar 21, 2017 •

edited

Loading