4
\$\begingroup\$

Please help me improving this code(golang), thanks!

Why I need built a API for xkcd

  • Can't get the data provided by xkcd directly in front-end(CORS).

  • So we can package the data to 10 comics per request. Instead of 1 request match one comic.

main.go

package main

import (
    "encoding/json"
    "fmt"
    "log"
    "net/http"
    "strconv"
    "strings"
)

type comicInfo struct {
    Number int    `json:"num"`
    Img    string `json:"img"`
    Title  string `json:"title"`
    Alt    string `json:"alt"`
}

var comicInfoMap = make(map[int]comicInfo)
var newestComic comicInfo

func fetch(url string, ch chan<- comicInfo) {
    if res, err := http.Get(url); err == nil {
        if res.StatusCode == http.StatusOK {
            newComic := comicInfo{}
            if err := json.NewDecoder(res.Body).Decode(&newComic); err == nil {
                fmt.Println(newComic)
                ch <- newComic
            }
        }
    }
}

func init() {
    ch := make(chan comicInfo)
    if res, err := http.Get("http://xkcd.com/info.0.json"); err == nil {
        if res.StatusCode == http.StatusOK {
            newComic := comicInfo{}
            if err := json.NewDecoder(res.Body).Decode(&newComic); err == nil {
                newestComic = newComic
                comicInfoMap[newestComic.Number] = newestComic
                for j := 0; j < 10; j++ {
                    addtion := 100 * j
                    for i := addtion + 1; i < addtion+101; i++ {
                        indexStr := strconv.Itoa(newComic.Number - i)
                        url := "http://xkcd.com/" + indexStr + "/info.0.json"
                        go fetch(url, ch)
                    }
                    for i := addtion + 1; i < addtion+101; i++ {
                        newComic := <-ch
                        comicInfoMap[newComic.Number] = newComic
                    }
                }
            }
        }
    }
}

func main() {
    http.HandleFunc("/", handler)
    log.Fatal(http.ListenAndServe(":9020", nil))
}

func handler(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Access-Control-Allow-Origin", "*")
    w.Header().Set("Access-Control-Allow-Methods", "GET, OPTIONS")
    w.Header().Set("Access-Control-Allow-Headers",
        "Origin, X-Requested-With, Content-Type, Accept")

    var encoder = json.NewEncoder(w)
    if r.URL.Path == "/" {
        encoder.Encode(newestComic)
    } else {
        res := strings.Split(r.URL.Path, "/")
        r.Body.Close()
        path, err := strconv.Atoi(res[1])
        if err != nil {
            return
        }
        if path < 100 {
            number := path
            newComicInfoSlice := []comicInfo{}
            for i := 0; i < 10; i++ {
                index := newestComic.Number - i - 10*number
                newComicInfoSlice = append(newComicInfoSlice, comicInfoMap[index])
            }
            encoder.Encode(newComicInfoSlice)
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Being inspired by recent xkcd comic I will review this particular piece of code:

res := strings.Split(r.URL.Path, "/")
path, err := strconv.Atoi(res[1])

As far as I understand it converts path string "/123" to int 123.

I see several issues here:

  • this code creates unnecessary (and possibly large) slice,
  • assumes that res[1] is always there,
  • parses negative numbers,
  • and allows input /123/whatever/goes/here/.

Split

Use strings.SplitN when you know how many parts of the slice you need. On /a/b/c/d/ input strings.Split will create slice with six items, but you need only res[1].

..

url.URL documentation states, that relative paths may omit leading slash. So on .. or . input access to res[1] will trigger panic with "runtime error: index out of range" message.

Malicious HTTP request may contain lots of crap similar ../.. so is crucial to validate this.

I suggest you to check that the path begins with a slash or test the length of res.

stronv.ParseUint

strconv.Atoi allows negative numbers. You can use strconv.ParseUint instead.


Path from HTTP request is same thing as any other form of user input. Proper handling of it is full of gotchas.

I suggest you to check that path begins with a slash and use strings.TrimSuffix to remove one possible trailing slash:

// Check that r.URL.Path begins with "/",
if (r.URL.Path[0] == "/") {
    // skip leading slash and trim one(!) possible trailing shash.
    num := strings.TrimSuffix(r.URL.Path[1:], "/")
    // Try to parse non negative number,
    tmp, err := strconv.ParseUint(num, 10, 32)
    if err != nil {
        // and return 404 if there is no one.
    }
    // Convert uint to int.
    index := int(tmp)

    // function body continues...
}
\$\endgroup\$
  • \$\begingroup\$ Thank u very much, your review is very helpful. \$\endgroup\$ – David Guan Oct 11 '18 at 2:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.