Repository: apostrophecms/sanitize-html (js)

Issue:
Image alt attribute with empty string should not be removed
PLEASE NOTE: make sure the bug exists in the latest patch level of the project. For instance, if you are running a 2.x version of Apostrophe, you should use the latest in that major version to confirm the bug.

## To Reproduce

Step by step instructions to reproduce the behavior:

```sh
npm install sanitize-html@2.11.0
```

```js
import sanitizeHtml from 'sanitize-html';

// EXPECT: <img alt="" src="https://example.com/" />
// ACTUAL: <img src="https://example.com/" />
console.log(
  sanitizeHtml('<img alt="" src="https://example.com/" />', {
    allowedAttributes: { img: ['alt', 'src'] },
    allowedTags: ['img']
  })
);

// EXPECT: <img alt="" src="https://example.com/" />
// ACUTAL: <img alt src="https://example.com/" />
console.log(
  sanitizeHtml('<img alt="" src="https://example.com/" />', {
    allowedAttributes: { img: ['alt', 'src'] },
    allowedTags: ['img'],
    nonBooleanAttributes: []
  })
);
```

## Expected behavior

`alt=""` should be left as-is.

## Describe the bug

Empty `alt` attribute means the alt text of the image is intentionally left blank. For example, border graphics may not need `alt` attribute if they are unimportant decoration.

When `alt` is an empty string, screen reader will skip the element and read nothing.

When `alt` is not set, screen reader will read "image".

So, `alt=""` is not same as `alt` or unset.

To workaround the issue, use `<img role="presentation" src="https://example.com/" />`.

## Details

**Version of Node.js:** 18.16.0
*PLEASE NOTE: Only stable LTS versions (10.x and 12.x) are fully supported but we will do our best with newer versions.*

**Server Operating System:** Ubuntu 20.04 on WSL
*The server (which might be your dev laptop) on which Apostrophe is running. Linux? MacOS X? Windows? Is Docker involved?*

**Additional context:** Seems regression of #529.
> Add any other context about the problem here. If the problem is specific to a browser, OS or mobile device, specify which.

**Screenshots**
*If applicable, add screenshots to help explain your problem.*

```sh
$ cat index.mjs
import sanitizeHtml from 'sanitize-html';

// EXPECT: <img alt="" src="https://example.com/" />
// ACTUAL: <img src="https://example.com/" />
console.log(
  sanitizeHtml('<img alt="" src="https://example.com/" />', {
    allowedAttributes: { img: ['alt', 'src'] },
    allowedTags: ['img']
  })
);

// EXPECT: <img alt="" src="https://example.com/" />
// ACUTAL: <img alt src="https://example.com/" />
console.log(
  sanitizeHtml('<img alt="" src="https://example.com/" />', {
    allowedAttributes: { img: ['alt', 'src'] },
    allowedTags: ['img'],
    nonBooleanAttributes: []
  })
);
$ node ./index.mjs
<img src="https://example.com/" />
<img alt src="https://example.com/" />
```

Pull request description:
Added 'allowedEmptyAttributes' option and kept empty 'alt' value by default

<!-- Please don't delete this template -->

<!-- PULL REQUEST TEMPLATE -->
<!-- (Update "[ ]" to "[x]" to check a box) -->

## Summary

*Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"*
Issue: #633 
Closes #633 
* Added allowedEmptyAttributes option with default attribute 'alt'
* Added check to keep empty attributes that specified in allowedEmptyAttributes option
* Added unit tests

## What are the specific steps to test this change?
run unit test - `npm run test`

*For example:*
> 1. Run the website and log in as an admin
> 2. Open a piece manager modal and select several pieces
> 3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
> 4. Check that all pieces have been archived properly

## What kind of change does this PR introduce?
*(Check at least one)*

- [X] Bug fix
- [ ] New feature
- [ ] Refactor
- [ ] Documentation
- [ ] Build-related changes
- [ ] Other

## Make sure the PR fulfills these requirements:

- [X] It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
- [X] The changelog is updated
- [X] Related documentation has been updated
- [X] Related tests have been updated

If adding a new feature without an already open issue, it's best to open a **feature request issue** first and wait for approval before working on it.

**Other information:**


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0e4b479..f18c988 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## UNRELEASED
+
+- Introduced the `allowedEmptyAttributes` option, enabling explicit specification of empty string values for select attributes, with the default attribute set to `alt`.
+
 ## 2.11.0 (2023-06-21)
 
 - Fix to allow `false` in `allowedClasses` attributes. Thanks to [Kevin Jiang](https://github.com/KevinSJ) for this fix!
diff --git a/index.js b/index.js
index b834977..bc62eeb 100644
--- a/index.js
+++ b/index.js
@@ -295,9 +295,11 @@ function sanitizeHtml(html, options, _recursing) {
             delete frame.attribs[a];
             return;
           }
-          // If the value is empty, and this is a known non-boolean attribute, delete it
+          // If the value is empty, check if the attribute is in the allowedEmptyAttributes array.
+          // If it is not in the allowedEmptyAttributes array, and it is a known non-boolean attribute, delete it
           // List taken from https://html.spec.whatwg.org/multipage/indices.html#attributes-3
-          if (value === '' && (options.nonBooleanAttributes.includes(a) || options.nonBooleanAttributes.includes('*'))) {
+          if (value === '' && (!options.allowedEmptyAttributes.includes(a)) &&
+            (options.nonBooleanAttributes.includes(a) || options.nonBooleanAttributes.includes('*'))) {
             delete frame.attribs[a];
             return;
           }
@@ -474,6 +476,8 @@ function sanitizeHtml(html, options, _recursing) {
             result += ' ' + a;
             if (value && value.length) {
               result += '="' + escapeHtml(value, true) + '"';
+            } else if (options.allowedEmptyAttributes.includes(a)) {
+              result += '=""';
             }
           } else {
             delete frame.attribs[a];
@@ -876,6 +880,9 @@ sanitizeHtml.defaults = {
     // these attributes would make sense if we did.
     img: [ 'src', 'srcset', 'alt', 'title', 'width', 'height', 'loading' ]
   },
+  allowedEmptyAttributes: [
+    'alt'
+  ],
   // Lots of these won't come up by default because we don't allow them
   selfClosing: [ 'img', 'br', 'hr', 'area', 'base', 'basefont', 'input', 'link', 'meta' ],
   // URL schemes we permit


Test patch:
diff --git a/test/test.js b/test/test.js
index c938b33..8fb651c 100644
--- a/test/test.js
+++ b/test/test.js
@@ -1616,4 +1616,37 @@ describe('sanitizeHtml', function() {
       nonBooleanAttributes: [ '*' ]
     }), '<input type="checkbox" />');
   });
+  it('should not remove empty alt attribute value by default', function() {
+    assert.equal(sanitizeHtml('<img alt="" src="https://example.com/" />', {
+      allowedAttributes: { img: [ 'alt', 'src' ] },
+      allowedTags: [ 'img' ]
+    }), '<img alt="" src="https://example.com/" />');
+  });
+  it('should convert the implicit empty alt attribute value to be an empty string by default', function() {
+    assert.equal(sanitizeHtml('<img alt src="https://example.com/" />', {
+      allowedAttributes: { img: [ 'alt', 'src' ] },
+      allowedTags: [ 'img' ]
+    }), '<img alt="" src="https://example.com/" />');
+  });
+  it('should not remove empty alt attribute value by default when an empty nonBooleanAttributes option passed in', function() {
+    assert.equal(sanitizeHtml('<img alt="" src="https://example.com/" />', {
+      allowedAttributes: { img: [ 'alt', 'src' ] },
+      allowedTags: [ 'img' ],
+      nonBooleanAttributes: []
+    }), '<img alt="" src="https://example.com/" />');
+  });
+  it('should not remove the empty attributes specified in allowedEmptyAttributes option', function() {
+    assert.equal(sanitizeHtml('<img alt="" src="" />', {
+      allowedAttributes: { img: [ 'alt', 'src' ] },
+      allowedTags: [ 'img' ],
+      allowedEmptyAttributes: [ 'alt', 'src' ]
+    }), '<img alt="" src="" />');
+  });
+  it('should remove all the empty attributes when an empty allowedEmptyAttributes option passed in', function() {
+    assert.equal(sanitizeHtml('<img alt="" src="https://example.com/" target="" />', {
+      allowedAttributes: { img: [ 'alt', 'src' ] },
+      allowedTags: [ 'img' ],
+      allowedEmptyAttributes: []
+    }), '<img src="https://example.com/" />');
+  });
 });