Panic when transform %(xxxxx)s value

[ryanyyxx]

Please give general description of the problem

in gokpg.ini/ini.v1/key.go
func (k *Key) transformValue(val string) string

When transform a %(xxxxx)s value, it's gonna to find the key xxxxx, then read key's value. But if the key is not found, read key's value will cause a nil pointer panic

Please provide code snippets to reproduce the problem described above

    nk, err := k.s.GetKey(noption)
    if err != nil || k == nk {
        // Search again in default section.
        nk, _ = k.s.f.Section("").GetKey(noption)
    }

    // Substitute by new value and take off leading '%(' and trailing ')s'.
    val = strings.Replace(val, vr, nk.value, -1)

Do you have any suggestion to fix the problem?

if the key is not found in default section, the function can return an empty string.

[unknwon]

Thanks for the feedback!

Please provide a sample INI content that could cause the panic.

[j-vizcaino]

Here is how to reproduce

package main

import (
  "github.com/go-ini/ini"
  "fmt"
)

func main() {
  f, err := ini.Load([]byte("[foo]\nbar = %(missing)s\n"))
  fmt.Printf("f: %v, err: %v\n", f, err)
  s := f.Section("foo")
  fmt.Println(s.Key("bar").String())
}

[unknwon]

I just pushed a fix, please re-pull the repository and help test!

[j-vizcaino]

Thanks for fixing the panic 🙇 The behaviour is unexpected, though, as the INI load would work but the returned value would be unusable by the application, as it lacks value expansion.

This is not a good solution, IMO, as this pushes the validation of the key to the reader code, while this could be caught by the library. Any idea how to make this better?

[unknwon]

The behaviour is unexpected, though, as the INI load would work but the returned value would be unusable by the application, as it lacks value expansion.

Another alternative I thought about is to return empty string "" for missing keys. WDYT?

[j-vizcaino]

Is a key with an empty value considered a valid INI syntax in the current implementation? If yes, then returning an empty string would be misleading to the caller who would be unable to distinguish a foo = from foo = %(missing)s

Maybe not a strong argument but returning an empty string is not great because errors frequently use %s instead of %q in messages, making it hard to troubleshoot for the operator.

Is there a stricter way to return an error, without breaking the current API?

[unknwon]

Maybe not a strong argument but returning an empty string is not great because errors frequently use %s instead of %q in messages, making it hard to troubleshoot for the operator.

The valid syntax is %(x)s, %s is not a subset of it, how is it making hard to troubleshoot?

[j-vizcaino]

Sorry for the unclear explanation, let me try again with an example.

Example

Consider the following code:

url := cfg.Section("database").Key("url").String()
if err := mydb.Connect(url); err != nil {
  return fmt.Errorf("Database %s connection failed: %w", url, err)
}
...

With INI

[database]
url = %(main_db_url)s

Now, say that url holds an invalid interpolated string in the config.

Option 1: returning the raw, non interpolated value

• Error message would read Database %(main_db_url)s connection failed: ...
• This is not great: the INI file is known to be invalid before even calling the DB connection call. This should have never reached the location in the code where the config value is even being used.
• This can be dangerous, as the interpolation can be interpreted somehow by the function that receives the value (potential injection)

Option 2: returning an empty string

• Error message would read Database connection failed: ...
• Note the double space here, since url is empty: this is not great for troubleshooting, as most readers would not realise the URL is empty and probably think the error log is useless, lacking context. Hence my comment regarding using %q instead of %s in the fmt.Errorf call above to surround the url value with quotes.

Next?

IMO, none of these are good solutions as they both have tradeoffs. The API, due to lazy loading it seems, makes it hard to report errors for strings keys, when interpolation is invalid.

Here are the things that came to mind (I hope that helps):

• Introduce StrictString() (string, error) and have the error bubble up to the caller. This implies users will need to update their code to handle that case properly
• Have GetKey return an error if the value interpolation fails? This might be unexpected though 🤔

If StrictString is the path to go, then String could return the non interpolated string. The user would probably be able to figure out the value is invalid.

WDYT?

[unknwon]

• Note the double space here, since url is empty: this is not great for troubleshooting, as most readers would not realise the URL is empty and probably think the error log is useless, lacking context.

I think this is the matter of how to better structure error message, instead of relying on underlying utility library to do the job. Like what you mentioned using %q instead of %s.

If StrictString is the path to go, then String could return the non interpolated string. The user would probably be able to figure out the value is invalid.

I agree with this approach.